Minor refactoring for simulator's sake #143
Conversation
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant S as Simulator
participant CS as ChainStore
participant B as Builder
S->>S: Check tip type
alt Tip is Origin
S->>B: Return builder unchanged
else Tip is Specific
S->>CS: Request header for tip
CS-->>S: Return header (or panic if missing)
S->>B: Update builder with the retrieved header
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
simulation/amaru-sim/src/bin/amaru-sim/simulator.rs (1)
166-172: Watch out for thepanic!.
If you’re feeling generous, you might bubble up an error rather than scaring the entire program stiff with a panic. Otherwise, she’ll be right for internal testing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/amaru-consensus/src/consensus/chain_selection.rs(10 hunks)crates/amaru-consensus/src/consensus/store.rs(2 hunks)simulation/amaru-sim/src/bin/amaru-sim/simulator.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build on windows-latest with target x86_64-pc-windows-msvc
- GitHub Check: Snapshots (preprod, 10.1.4)
🔇 Additional comments (17)
crates/amaru-consensus/src/consensus/store.rs (2)
15-15: Cheerio! ImportingPointis spot-on.
No concerns here, old chap. This import is crucial for referencing the origin in case we run out of parents.
115-115: Double-check the shift from error to default.
Swapping an error forPoint::Originmight be all right if you want to keep the chain going when no parent is found. However, do confirm you won’t be silently glossing over genuine issues.simulation/amaru-sim/src/bin/amaru-sim/simulator.rs (1)
161-165: Neat usage of the'alifetime parameter.
Blimey, that’s a tidy approach to letting the function hand back the samebuilderwith a matching lifetime, saving you from lifetimes mischief.crates/amaru-consensus/src/consensus/chain_selection.rs (14)
16-17: Additional imports look grand.
All good here, mate. A tidy extension to the toolbox for handlingPointandHASH_SIZE.
31-31: Top-notch pivot toTip<H>for the anchor field.
Switching the anchor toTip<H>clarifies whether we’re anchored atGenesisor a specific header. Good on you!
40-40:start_fromwithTip<H>is a sweet shift.
This function elegantly sets up a fragment from eitherGenesisor an actual header. G’day for flexibility!
57-59: Methodtip()now returns aTip<H>.
You’re neatly distinguishing betweenGenesisand a live header. That’s the bee’s knees for chain clarity.
75-79: The newTip<H>enum is positively spiffing!
This is an excellent approach to differentiateGenesisfrom an actual header. Keep calm and tip on.
81-95: EncodingTip<H>via CBOR.
Your encode logic clearly handles eitherGenesisorHdrscenario. Jolly fine, ensures consistent serialisation.
97-125: ImplementingIsHeaderforTip<H>.
Crikey, that’s cunning! It means anywhere a header is required,Tip<H>can fill the role. A brilliant bit of polymorphism.
128-134: Converting fromOption<H>toTip<H>.
This is a neat way to avoid unwrapping. If you lack a real header, you getGenesis. Easy as pie!
212-212: Using.into()to convert toTip<H>in the builder.
All gravy here, mate. A direct transformation is quite tidy.
219-219:Fragment::start_fromgets aTip<H>?
Spreading the usage ofTip<H>fosters consistency. It’s marvellous to see it in the fragment’s starting point.
271-271:self.tip = Tip::Hdr(header);
Right as rain. Assigning a real header to the chain’s tip is perfectly correct for the roll-forward scenario.
309-309: Set tip toHdr(best_tip)on rollback.
When the chain’s rolled back and you find a best chain, you properly set it. Lovely stuff, no complaints here.
320-329: Caution with thatpanic!("Fragment has no tip").
We see the slip: height > 0 implies there must be a header. A compile-time check would be smashing, but for now at least you know where to step on the brake if something goes awry.
549-549: Asserting the tip is indeedHdr(*rollback_point).
This test checks that your rollback logic does what it says on the tin. Good on you!
This is useful in testing when we start following a chain from scratch. To work, this also requires cooperation from epoch_from_slot() which currently hard codes epoch lengths which obviously won't work for small slot values. Signed-off-by: Arnaud Bailly <[email protected]>
This means the chain selector starts with tip being 'Genesis' Signed-off-by: Arnaud Bailly <[email protected]>
abailly
force-pushed
the
abailly/refactoring-for-simulator
branch
from
1acd5ec to
2bf1415
Compare
This small PR introduces 2 changes in order to ease full consensus testing with "simulator". Arguably, those changes are useful and make sense in and of themselves.
Summary by CodeRabbit
New Features
Refactor