Simplex reconfiguration framework - Part IV (Address code review comments for initial draft PR)#366
Open
Simplex reconfiguration framework - Part IV (Address code review comments for initial draft PR)#366
Conversation
This commit adds some helpers for the Simplex reconfiguration framework. This commit entails: - README.md changes to align with latest implementation - misc.go contains helpers which will be removed once we moved the code to avalanchego. - builds_decision.go contains a wrapper to the mempool that also listens to P-chain changes, and returns either when the mempool needs to build a block or re-configuration is in order. - encoding.go contains the types that encode Simplex metadata and blocks. Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
…ions) - Add some structs and interface definitions - Add some utility methods to be used in the next commit. Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
- Add block building to msm.go - Add verification.go which contains logic for block verification - Add tests that mimic Simplex flow (fake_node_test.go) Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
Addresses review comment: parameter was threaded through but never read. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses review comment: field was only written to, never read. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses review comment: the returned VMBlock was unused after dropping innerChain. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses review comment: clarify that act randomly chooses between finalizing, building, or no-op. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses review comment. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses review comment: test names should describe what is being tested (state machine epoch transitions), not the test fixture. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses review comment: the type's purpose is to drive the state machine across epoch transitions, so a more descriptive name is used. The file is renamed accordingly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses review comments: finalized blocks were a prefix of notarized blocks, so keeping two separate slices was redundant and error prone (e.g. tryFinalizeNextBlock panicked when the two went out of sync). Replaces both with a single blocks []blockState slice where each entry carries a finalized flag. Also removes the duplicate lookup in the GetBlock test fixture that would return finalized blocks as non-finalized. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses review comment: a zero value should not match a valid variant, so callers get a clear error if an uninitialized value sneaks through. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses review comment: inline the equality loop with slices.EqualFunc and share the sort comparator. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses review comment: the ForEach/SumWeights wrappers added unnecessary indirection. Inline them with ordinary for loops in encoding.go, msm.go, and verification.go. Filter predicates no longer need the unused index parameter, and ForEach/SumWeights themselves are deleted along with their dedicated tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses review comment: identifyCurrentState didn't belong on StateMachine and never actually returned an error, so hoist it onto SimplexEpochInfo and simplify the return. Callers and the existing test are updated accordingly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses review comment: a lazy-init pattern based on a sticky 'initialized' bool hid the dependency between field assignment and first-call wiring. Introduces NewStateMachine(config) that eagerly wires verifiers and returns *StateMachine. NewStateMachine wires the verifiers through closures over the returned *StateMachine, so callers (including tests) can keep mutating fields like sm.GetBlock, sm.GetValidatorSet, and sm.GetPChainHeight after construction and the verifiers will see the fresh values. Verifier types are unchanged. The test helper newStateMachine now returns *StateMachine. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses review comment: moves the block-building decider wiring out of StateMachine.createBlockBuildingDecider into a package-level newBlockBuildingDecider alongside the decider type itself. This keeps the decider's initialization next to the decider, not the state machine. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses review comment: the childBlock VMBlock parameter was always nil at the call site and immediately overwritten by BuildBlock. Replace the blockBuildingDecision value with a plain transitionEpoch bool so the function signature spells out what actually matters. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses review comment: simplexBlacklist doesn't need to be a pointer to express 'none' — the zero value already does — and the simplex prefix is redundant because the type itself (simplex.Blacklist) carries that context. Callers now pass a simplex.Blacklist value, the encoded bytes are always written to the block (even when empty), and tests use the emptyBlacklistBytes helper when they expect the no-op encoding. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses review comment: both are self-contained and not specifically about the StateMachine wiring. Relocating them out of msm.go keeps that file focused on the state machine itself. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses review comment: msm_test.go was cluttered with the fakeVMBlock, blockStore, signatureVerifier/aggregator, validatorSetRetriever, keyAggregator, blockBuilder stubs plus newStateMachine/testConfig. They are test plumbing, not test cases, so relocate them to helpers_test.go to make the actual tests easier to navigate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses review comment: buildBlockNormalOp / buildBlockCollectingApprovals / buildBlockEpochSealed / buildBlockImpatiently / createSealingBlock / wrapBlock all took simplexMetadata bytes plus a separately drilled prevBlockSeq. Replace both with a single simplex.ProtocolMetadata argument; prevBlockSeq is now derived locally as metadata.Seq - 1, and wrapBlock serialises the metadata when building the block. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
yacovm
changed the title
yacovm
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.