8000 chore(consensus): Port Blob Defaults Code by alesforz · Pull Request #18 · alesforz/cometbft · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

chore(consensus): Port Blob Defaults Code #18

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 33 commits into
base: alesforz/feature/blobs
Choose a base branch
from
Open

chore(consensus): Port Blob Defaults Code #18

wants to merge 33 commits into from

Conversation

alesforz
Copy link
Owner

No description provided.

jmalicevic and others added 16 commits May 27, 2025 14:36
@jmalicevic @alesforz
Added maxBytes param update to e2e
835b1fa
@jmalicevic @alesforz
Update ci.toml with proper blob_max_bytes value and fix check
287019a
@jmalicevic @alesforz
fixed edge case when blobs are disabled, lowered the height they are …
9eeb967 
…activated at
@jmalicevic @alesforz
Minor fixes to address initial comments
4629668
@jmalicevic @alesforz
BlobMaxBytes update logic is the same as VE
889ff89
@jmalicevic @alesforz
Removed printf
70ac3b4
@jmalicevic @alesforz
Moved BlobMaxBytes var close to BlobMaxBytesUpdateHeight
697b225
@alesforz
fix merge oddities and run linter
2e11172
@jmalicevic @alesforz
Do not use blob in UTs
024f1f5
@jmalicevic @alesforz
Fix UT to use blobs and set maxBlobBytes to 800kb
b45e63f
@jmalicevic @alesforz
Fix typo
69a4591
@jmalicevic @alesforz
Applied review comments
5907529
@jmalicevic @alesforz
Fix UTs
c11617b
@alesforz
manually add changes that cherry-pick failed to add
cbe1b85
@jmalicevic < 8000 a class="avatar avatar-user" style="width:20px;height:20px;" data-test-selector="commits-avatar-stack-avatar-link" data-hovercard-type="user" data-hovercard-url="/users/sergio-mena/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="/sergio-mena"> @sergio-mena @alesforz
Update comment
4688876 
Co-authored-by: Sergio Mena <sergio@informal.systems>
@jmalicevic @sergio-mena @alesforz
Update comment
c513c2f 
Co-authored-by: Sergio Mena <sergio@informal.systems>
@alesforz alesforz self-assigned this May 28, 2025
@alesforz alesforz requested a review from jmalicevic May 28, 2025 13:11
@alesforz alesforz changed the title Port Blob Defaults Code chore(consensus): Port Blob Defaults Code May 28, 2025
jmalicevic
jmalicevic reviewed May 28, 2025
View reviewed changes
internal/consensus/byzantine_test.go
@@ -378,6 +372,7 @@ func TestByzantineConflictingProposalsWithPartition(t *testing.T) {
require.NoError(t, err)

conR := NewReactor(css[i], true) // so we don't start the consensus states
conR.conS.state.ConsensusParams.Blob.MaxBytes = types.MaxBlobSizeBytes
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might not be needed as here by default the size is not 0, but it does not hurt.

jmalicevic
jmalicevic reviewed May 28, 2025
View reviewed changes
jmalicevic
jmalicevic reviewed May 28, 2025
View reviewed changes
@alesforz
Copy link
Owner Author
alesforz commented Jun 3, 2025 •
edited
Loading

I reverted the commits bumping Go's version, because I think this should be a decision of Berachain's team (and pushed in a separate PR).

@alesforz
Copy link
Owner Author
alesforz commented Jun 6, 2025 •
edited
Loading

e2e error below. We do enable PBTS in the ci.toml, so I'm not sure why this happens.

validator04  | E[2025-06-06|14:16:35.513] CONSENSUS FAILURE!!!    module=consensus err="PBTS has to be enabled"
stack:
goroutine 153 [running]:
    runtime/debug.Stack()
        runtime/debug/stack.go:26 +0x67
    github.com/cometbft/cometbft/internal/consensus.(*State).receiveRoutine.func2()
        github.com/cometbft/cometbft/internal/consensus/state.go:861 +0x58
    panic({0x1eed680?, 0x24b5eb0?})
        runtime/panic.go:785 +0x132
    github.com/cometbft/cometbft/state.State.MakeBlock(
        { {0xb, 0x1}, {0xc0005c247b, 0x5} },
        {0xc0005c248e, 0x2},
        0x1,
        0x0,
        { {0x0, 0x0, ...}, ... },
        ...
    )
        github.com/cometbft/cometbft/state/state.go:257 +0x685
    github.com/cometbft/cometbft/state.(*BlockExecutor).CreateProposalBlock(
        _,
        {_, _},
        _,
        { {0xb, 0x1}, {0xc0005c247b, 0x5} },
        {0xc0005c248e, 0x2},
        ...,
    )
        github.com/cometbft/cometbft/state/execution.go:142 +0x5e5
    github.com/cometbft/cometbft/internal/consensus.(*State).createProposalBlock(
        0xc000285c08,
        {0x24cab78, 0x401b260}
    )
        github.com/cometbft/cometbft/internal/consensus/state.go:1535 +0x798
    github.com/cometbft/cometbft/internal/consensus.(*State).defaultDecideProposal(
        0xc000285c08,
        0x1,
        0x0
    )
        github.com/cometbft/cometbft/internal/consensus/state.go:1336 +0xe5
    github.com/cometbft/cometbft/internal/consensus.(*State).enterPropose(
        0xc000285c08,
        0x1,
        0x0
    )
        github.com/cometbft/cometbft/internal/consensus/state.go:1312 +0x119e
    github.com/cometbft/cometbft/internal/consensus.(*State).enterNewRound(
        0xc000285c08,
        0x1,
        0x0
    )
        github.com/cometbft/cometbft/internal/consensus/state.go:1222 +0x14f0
    github.com/cometbft/cometbft/internal/consensus.(*State).handleTimeout(
        _,
        {_, _, _, _},
        {0x1, 0x0, 0x1, {0x1e286186, 0xedfd4ef42, ...}, ...}
    )
        github.com/cometbft/cometbft/internal/consensus/state.go:1088 +0xc9e
    github.com/cometbft/cometbft/internal/consensus.(*State).receiveRoutine(
        0xc000285c08,
        0x0
    )
        github.com/cometbft/cometbft/internal/consensus/state.go:925 +0xa5f
created by github.com/cometbft/cometbft/internal/consensus.(*State).OnStart in goroutine 116
    github.com/cometbft/cometbft/internal/consensus/state.go:406 +0x22b

@alesforz
Copy link
Owner Author

I found the issue: a sneaky merge added fields to the Testnet struct that were already defined in its embedded Manifest struct. As a result, at runtime, accessing something like testnet.InitialHeight read the uninitialized Testnet.InitialHeight instead of Manifest.InitialHeight. In other words, the values set on the manifest were ignored because the duplicate fields shadowed them.

@alesforz
Copy link
Owner Author

Ran the nightlies. The unit tests ofnetworks/nightly/gen-group02-0001.toml failed, everything else passes.

The errors:

FAIL: TestApp_Tx (204.06s)
    --- FAIL: TestApp_Tx/full02 (60.00s)
        e2e_test.go:64: 
            	Error Trace:	/Users/alesforz/workspace/forks/cometbft-alesforz/test/e2e/tests/app_test.go:98
            	            				/Users/alesforz/workspace/forks/cometbft-alesforz/test/e2e/tests/e2e_test.go:64
            	Error:      	Condition never satisfied
            	Test:       	TestApp_Tx/full02
            	Messages:   	submitted tx (E6EC3F4D86C9644CE3B039FBD8C369333822AEB76A5DBC517BD5F78ED84B961F) wasn't committed after 1m0s
    --- FAIL: TestApp_Tx/full03 (60.00s)
        e2e_test.go:64: 
            	Error Trace:	/Users/alesforz/workspace/forks/cometbft-alesforz/test/e2e/tests/app_test.go:98
            	            				/Users/alesforz/workspace/forks/cometbft-alesforz/test/e2e/tests/e2e_test.go:64
            	Error:      	Condition never satisfied
            	Test:       	TestApp_Tx/full03
            	Messages:   	submitted tx (68C4D6EE6D43ADC332D93A6BE1A972C14CF804F5F84E59D279D6AE6868A93547) wasn't committed after 1m0s
    --- FAIL: TestApp_Tx/validator05 (60.00s)
        e2e_test.go:64: 
            	Error Trace:	/Users/alesforz/workspace/forks/cometbft-alesforz/test/e2e/tests/app_test.go:98
            	            				/Users/alesforz/workspace/forks/cometbft-alesforz/test/e2e/tests/e2e_test.go:64
            	Error:      	Condition never satisfied
            	Test:       	TestApp_Tx/validator05
            	Messages:   	submitted tx (281CFAC88A9FF1482CD0FAF35617F04D989CB3653201A8B7B8CE99FFA292D25A) wasn't committed after 1m0s
--- FAIL: TestEvidence_Misbehavior (0.03s)
    evidence_test.go:20: 
        	Error Trace:	/Users/alesforz/workspace/forks/cometbft-alesforz/test/e2e/tests/evidence_test.go:20
        	Error:      	Not equal: 
        	            	expected: 1
        	            	actual  : 0
        	Test:       	TestEvidence_Misbehavior
        	Messages:   	difference between the amount of evidence produced and committed
--- FAIL: TestValidator_Propose (0.01s)
    validator_test.go:67: 
        	Error Trace:	/Users/alesforz/workspace/forks/cometbft-alesforz/test/e2e/tests/e2e_test.go:168
        	            				/Users/alesforz/workspace/forks/cometbft-alesforz/test/e2e/tests/validator_test.go:67
        	Error:      	Received unexpected error:
        	            	post failed: Post "http://127.0.0.1:5731/v1": dial tcp 127.0.0.1:5731: connect: connection refused
        	Test:       	TestValidator_Propose
--- FAIL: TestValidator_Sign (0.01s)
    validator_test.go:107: 
        	Error Trace:	/Users/alesforz/workspace/forks/cometbft-alesforz/test/e2e/tests/e2e_test.go:168
        	            				/Users/alesforz/workspace/forks/cometbft-alesforz/test/e2e/tests/validator_test.go:107
        	Error:      	Received unexpected error:
        	            	post failed: Post "http://127.0.0.1:5731/v1": dial tcp 127.0.0.1:5731: connect: connection refused
        	Test:       	TestValidator_Sign
FAIL
FAIL	github.com/cometbft/cometbft/test/e2e/tests	334.731s
FAIL

@alesforz
Set manifest.BlobMaxBytes to types.MaxBlobSizeBytes if
bc63a75 
`manifest.BlobMaxBytesUpdateHeight` < 0.

Without this change, if `manifest.BlobMaxBytesUpdateHeight` is -1,
`manifest.BlobMaxBytes` would be set to 0, thus making the nightlies fail
with a "BlobMaxBytes cannot be 0" error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmalicevic jmalicevic jmalicevic left review comments

Assignees

@alesforz alesforz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alesforz @jmalicevic
0