-
Notifications
You must be signed in to change notification settings - Fork 699
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.
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
[Proposal] - Version Proposal Blocks #2439
Conversation
ba8df67
to
d88d397
Compare
d88d397
to
0632c28
Compare
2c4e517
to
41be5b9
Compare
// Note: we assume this block's sibling doesn't | ||
// need the parent's state when it's rejected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: check whether this comment is still true
s.EXPECT().SetHeight(blk.Height()).Times(1), | ||
s.EXPECT().AddStatelessBlock(blk).Times(1), | ||
|
||
onAcceptState.EXPECT().Apply(gomock.Any()).Return(nil), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notably commit is not called here. Deferred to the accepted option
@@ -121,7 +126,7 @@ func (e *ProposalTxExecutor) AddValidatorTx(tx *txs.AddValidatorTx) error { | |||
|
|||
onAbortOuts, err := verifyAddValidatorTx( | |||
e.Backend, | |||
e.OnCommitState, | |||
e.OnProposalBlockState, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in all of these verify methods OnProposalBlockState
should never be modified. Consider carving out of state.Chain
interface into a state.ReadOnlyChain
containing only getters. It won't actually enforce any constness of proposalBlockState, but it'd be more expressive.
Deferred to a future PR to keep scope reduced.
// | ||
// Invariant: Both [OnCommitState] and [OnAbortState] are built on | ||
// top of OnProposalBlockState when provided to the executor. | ||
OnProposalBlockState state.Diff |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we used to lookup state quantities from OnCommitState. Imma change this to do lookups/verifications from OnProposalBlockState
// Consume the input UTXOs whether we accept commit or abort blocks | ||
avax.Consume(e.OnProposalBlockState, tx.Ins) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this probably blurs a bit the distinction of what state changes go in proposal block state and what goes in options block states.
However it should be marginally more efficient to do this once, in proposal block state, rather than twice in options blocks.
There seems to be an issue around block building. See logs from a mainnet sync with partial sync activated. Looking at this.
|
@@ -117,57 +117,71 @@ func (a *acceptor) ApricotAtomicBlock(b *block.ApricotAtomicBlock) error { | |||
} | |||
|
|||
func (a *acceptor) abortBlock(b block.Block, blockType string) error { | |||
parentID := b.Parent() | |||
parentState, ok := a.blkIDToState[parentID] | |||
blkID := b.ID() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when we verified the option we copied over option's blockState the preference. Hence we can lookup the option blockState now, instead of its parent's one
acceptor.backend.blkIDToState[blkID] = &blockState{ | ||
onAcceptState: onAcceptState, | ||
{ | ||
err = acceptor.BanffCommitBlock(blk) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated to BanffCommitBlock here. There is not much difference in the logic and it's better to test options currently created on mainnet rather than deprecated versions
So failure were happening when proposal block was the accepted one; it's parent block state was missing when trying to iterator over current stakers. Note that the iteration is done by recursively fetching all diffs till state.State and building the iterator by merging these diffs.
This is wrong because when applying the proposal block I was overwriting the last Accepted block ID.
This is fixed by deferring proposal state application to when option block is accepted. |
Superseded by #2451 |
Why this should be merged
This is a refactoring to enable decision txs inclusion in proposal blocks.
For now we just change the way we store proposal blocks state changes in memory.
Specifically we store all changes common to both commit and abort options (timestamp advance time post fork, inputs consumption of the proposal tx) in proposal block state and we explicitly index proposal block state by itself.
Previously no change was index under proposal block; all changes were indexed under the options.
Followed by #2442
How this works
Split changes related to proposal + options blocks into two chucks:
Changes are still kept in memory while block is verified; they are flushed to state and stored to disk when the option is accepted so that proposal block and accepted option changes are stored atomically.
How this was tested
CI + mainnet sync