fix(consensus): more robust catch up sync - #1646
Conversation
WalkthroughRemoved NFT Changes
Sequence Diagram(s)sequenceDiagram
participant Worker
participant Pacemaker
participant Inbound as OnInboundMessage
participant CatchUp as OnCatchUpSync
Note over Worker,Pacemaker: LeaderTimeout propagation and catch-up trigger
Worker->>Pacemaker: on_leader_timeout(LeaderTimeout{current_height, current_high_pc, num_timeouts})
Pacemaker->>Worker: publish LeaderTimeout
Worker->>Inbound: next_message(current_epoch, current_height, has_processed_first_block)
Inbound->>Inbound: msg_relative_view(..., has_processed_first_block)
alt missing justify / need catch-up
Worker->>CatchUp: request_catch_up_sync(start_height)
CatchUp->>Worker: send per-block proposals (CatchUpRequestMessage -> Proposal)
end
sequenceDiagram
participant OnReceiveVote
participant PaceMakerHandle
Note over OnReceiveVote: reset_leader_timeout now accepts HighPc ref
OnReceiveVote->>OnReceiveVote: compute high_pc
OnReceiveVote->>PaceMakerHandle: reset_leader_timeout(&high_pc)
PaceMakerHandle->>PaceMakerHandle: derive high_pc.height() and send reset
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Areas needing extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
270f76c to
68ca1ec
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/consensus/src/traits/certificate.rs (1)
65-76: Correct the misleading comment about when last_sent_new_view is cleared.The comment claims: "If we receive a new high PC, clear the last sent new view," but this code executes when the received PC is not new (
high_pc.height() >= self.height()). The log message itself confirms: "not new".The actual scenario is: when the stored high PC is already at or above this certificate's height (no progress),
last_sent_new_viewis cleared. This edge case represents: the chain progressed without us while we were offline/silent, so any previous NEWVIEWs are now stale and should be cleared before sending new ones.Update the comment to accurately describe this case instead of describing a "new high PC" scenario (which occurs in the
Some(_) | Nonebranch, where the call is NOT made).crates/consensus/src/hotstuff/worker.rs (1)
1071-1159: Catch-upexpected_batch_heightcan prevent leaving catch-up when remote height is closeThe new
CatchUptracking is a useful addition, but there’s a subtle liveness issue in howexpected_batch_heightis initialized:self.worker_state.catch_up = Some(CatchUp { high_qc: remote_height, // we get batches of 100 blocks which can only justify up to view h + 99 expected_batch_height: current_height + NodeHeight(99), });
CatchUp::set_next_batchcorrectly clamps subsequent batches tohigh_qc:self.expected_batch_height = (current_height + NodeHeight(99)).min(self.high_qc);However, the initial
expected_batch_heightis not clamped. Ifremote_height(high_qc) is less thancurrent_height + 99(for example, you’re only a few views behind), then:
current_heightcan never reachexpected_batch_height, so theif current_height >= catch_up.expected_batch_heightbranch inon_proposal_messagenever fires.set_next_batchis never called.catch_upis never cleared, leavingworker_state.is_catching_up()permanentlytrue, which causespropose_now(and leader-timeout catch-up requests) to be permanently suppressed even after we are fully caught up.This can leave a node stuck in “catching up” mode whenever the remote is ahead by fewer than 100 views.
A straightforward fix is to clamp the initial batch target to
remote_heightas well:- self.worker_state.catch_up = Some(CatchUp { - high_qc: remote_height, - // we get batches of 100 blocks which can only justify up to view h + 99 - expected_batch_height: current_height + NodeHeight(99), - }); + self.worker_state.catch_up = Some(CatchUp { + high_qc: remote_height, + // we get batches of 100 blocks which can only justify up to view h + 99, + // but never beyond the remote high QC we are trying to reach. + expected_batch_height: (current_height + NodeHeight(99)).min(remote_height), + });As a minor clean-up, the later
if self.worker_state.catch_up.is_some()check is redundant with the earlierif self.worker_state.is_catching_up()guard and could be removed to reduce confusion.Also applies to: 1247-1257
🧹 Nitpick comments (11)
applications/tari_walletd/web_ui/src/routes/AssetVault/NFTs/components/NftParts.tsx (1)
151-155: Consider accessibility for metadata display.The metadata is rendered as plain
<div>elements without semantic structure. For better accessibility and consistency with Material-UI patterns, consider using Typography components or a proper list structure.Example refactor:
- {metadata.map(([key, value], index) => ( - <div key={index}> - <strong>{key}:</strong> {value} - </div> - ))} + {metadata.map(([key, value], index) => ( + <Box key={index} sx={{ mb: 0.5 }}> + <Typography component="span" variant="body2" fontWeight="bold"> + {key}: + </Typography>{" "} + <Typography component="span" variant="body2"> + {value} + </Typography> + </Box> + ))}crates/consensus_types/src/certificates/quorum_certificate.rs (2)
47-61: Consider more concise accessor patterns.The current implementation is correct, but could be slightly more idiomatic:
pub fn as_proposal_certificate(&self) -> Option<&ProposalCertificate> { - if let Self::ProposalCertificate(pc) = self { - Some(pc) - } else { - None - } + match self { + Self::ProposalCertificate(pc) => Some(pc), + _ => None, + } } pub fn as_timeout_certificate(&self) -> Option<&TimeoutCertificate> { - if let Self::TimeoutCertificate(tc) = self { - Some(tc) - } else { - None - } + match self { + Self::TimeoutCertificate(tc) => Some(tc), + _ => None, + } }
63-77: Consider addingFromtrait implementations for better ergonomics.While the consuming accessors work correctly, consider adding
Fromimplementations to make conversion from inner types more convenient:impl From<ProposalCertificate> for QuorumCertificate { fn from(pc: ProposalCertificate) -> Self { Self::ProposalCertificate(pc) } } impl From<TimeoutCertificate> for QuorumCertificate { fn from(tc: TimeoutCertificate) -> Self { Self::TimeoutCertificate(tc) } }This would allow callers to use
.into()for conversions, which is more idiomatic than wrapping in the enum variant directly.crates/consensus_tests/src/support/harness.rs (1)
625-669: Config defaults andmodify_configAPI are reasonable, with minor extensibility noteBumping
state_tree_cleanup_intervalandepoch_gc_intervalto 1000s is a pragmatic way to avoid cleanup/gc interfering with relatively short tests. The newmodify_configmethod cleanly complementsmodify_consensus_constantsfor more advanced tuning. If you foresee frequent per-test toggles (like debugging flags), consider adding small helper methods (e.g.with_debugging_data_enabled()) onTestBuilderto reduce repeated closure boilerplate, but this is optional.crates/consensus_tests/src/support/network.rs (1)
112-151: Address-based offline handling is consistent; consider deduping offline entriesThe refactor to keep
offline_destinations: Arc<RwLock<Vec<TestAddress>>>and to gate both broadcast and leader messages viais_offline_destinationgives a clear, symmetric notion of “offline” at the address level and aligns with the newgo_offline/go_online/is_offlinesurface. Behavior (dropping messages if either side is offline, except self-messages) matches test expectations.One small improvement:
go_offlineblindly pushes the node, so repeated calls may accumulate duplicates and slightly increase iteration cost inis_offline/is_offline_destination. Not a correctness issue, but you could optionally guard against duplicates or switch to aHashSetfor cleaner semantics.Also applies to: 175-181, 222-223, 327-379
crates/consensus_tests/src/leader_failure.rs (1)
4-4: New catch-up test and offline API usage look correct; timing-based control is acceptable but brittleSwitching the existing tests to
test.network().go_offline(failure_node.clone()).awaitmatches the new address-based network API and keeps the semantics unchanged.The new
single_shard_node_goes_down_and_catches_uptest is well-structured:
- It disables eviction via
modify_configso the “failed” node is expected to recover.- Uses
Instantand elapsed durations to bracket when the node goes offline and comes back online.- Sends additional transactions around both transitions, then waits until all validators, including the previously offline one, have committed every recorded
tx_id.- Retains the existing safety net on
committed_height > NodeHeight(50)to detect stalls.One caveat: the wall-clock timing thresholds (2s and 13s) introduce some inherent flakiness risk under heavy CI load. If this ever becomes an issue, you could tighten the test by keying offline/online transitions to block heights and/or explicit counters instead of elapsed time. For now, it’s acceptable given the generous timeout and pacemaker configuration.
Also applies to: 41-41, 115-117, 190-190, 309-309, 354-426
applications/tari_validator_node/src/bootstrap.rs (1)
217-221: Consider making debugging data configurable.The TODO comment indicates this should be configurable in the future. Since debugging data may increase storage requirements and impact performance, consider prioritizing making this configurable through the application config rather than enabling it unconditionally in production.
You could add a field to
ConsensusConfigorApplicationConfig:pub struct ConsensusConfig { // ... existing fields pub enable_debugging_data: bool, }Then use it here:
let state_store = ValidatorNodeStateStore::open( &config.validator_node.state_db_path, - // TODO: just enable it always for now, later make it configurable and default to true for testnets - DatabaseOptions::default().with_debugging_data(true), + DatabaseOptions::default().with_debugging_data( + config.validator_node.consensus.enable_debugging_data + ), )?;crates/state_store_rocksdb/src/reader.rs (1)
590-599: Consider the performance trade-off of the existence check guard.The new guard prevents querying transaction executions that aren't finalized, which is good defensive coding. However, this adds an extra database read (
exists()check) for every call, including the common case where the transaction is actually finalized.For finalized transactions (the typical case), you now perform:
exists()check on FinalizedTransactionLinkCf- Query on ByTransactionIdQuery
- Get on BlockTransactionExecutionCf
vs. the original two operations (2 and 3).
If profiling shows this is on a hot path and the extra read impacts performance for the common case, consider whether the iterator check at line 602 already provides sufficient "not found" handling without the upfront exists check.
crates/consensus/src/hotstuff/worker.rs (2)
245-267: Genesis handling andhas_processed_first_blockinitializationWiring
create_genesis_block_if_requiredto returnbooland flippingworker_state.has_processed_first_block = !is_genesismakes sense for the “fresh epoch just created” case and aligns with how the first-block gating is used in inbound-message routing.One thing to double‑check: on a node restart where the epoch’s genesis already exists but the node has still never processed a height‑1 proposal,
is_genesiswill befalseandhas_processed_first_blockwill start astrue. If the “first block parked” bug can occur across restarts (not only on first startup), you may want to derive this flag from persisted state (e.g. presence of executed/voted height > 0) instead of only from whether genesis was freshly created.Also applies to: 1162-1208
831-862: Propose gating with catch-up state andLastSentVoteThe early return in
propose_nowwhenworker_state.is_catching_up()istruemakes sense: while replaying remote state we should not create new proposals.The additional guard against proposing when
LastSentVote.block_height() >= next_heightalso helps avoid unnecessary (or conflicting) proposals once we’ve already voted at or beyond the intended height. This should reduce redundant traffic without harming liveness, assumingLastSentVoteis always persisted when a vote is sent.crates/consensus/src/hotstuff/on_inbound_message.rs (1)
123-147: Proposalmsg_relative_viewlogic and first-block special caseThe updated
msg_relative_viewforProposalmessages captures several important behaviors:
- Proposals from past epochs or whose QC/block heights are strictly behind the current view are classified as
Past.- Proposals from future epochs are now immediately
Discard, which is consistent with them being invalid for the current epoch.- When
has_processed_first_block == true, proposals withpc_height > next_heightare treated asFuture, keyed by eitherpc_heightorpc_height + 1if a timeout certificate is present.- When
has_processed_first_block == false, the additional special casing:
(pc_height == 1 && block_height == 2)→Future { height: 1 }, and- any
block_height > 1→Future { height: pc_height },
ensures that second (and later) blocks are buffered instead of immediately processed while the first block is still outstanding.This is a targeted but effective way to handle the “first block parked” scenario; the TODO comment calling it “hacky” is fair, but the behavior matches the bug description and should be robust as long as the early views (0/1/2) semantics remain unchanged.
Also applies to: 179-247
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (55)
applications/tari_indexer/web_ui/src/routes/VN/Components/NftRow.tsx(0 hunks)applications/tari_indexer/web_ui/src/routes/VN/Components/Resources.tsx(1 hunks)applications/tari_validator_node/log4rs_sample.yml(2 hunks)applications/tari_validator_node/src/bootstrap.rs(1 hunks)applications/tari_validator_node/src/p2p/rpc/mod.rs(1 hunks)applications/tari_validator_node/src/p2p/rpc/rpc_impl.rs(1 hunks)applications/tari_validator_node/src/p2p/rpc/state_sync_task.rs(1 hunks)applications/tari_walletd/web_ui/src/routes/AssetVault/NFTs/components/NftParts.tsx(2 hunks)crates/consensus/src/hotstuff/block_change_set.rs(3 hunks)crates/consensus/src/hotstuff/common.rs(1 hunks)crates/consensus/src/hotstuff/error.rs(1 hunks)crates/consensus/src/hotstuff/on_catch_up_sync.rs(3 hunks)crates/consensus/src/hotstuff/on_catch_up_sync_request.rs(5 hunks)crates/consensus/src/hotstuff/on_inbound_message.rs(5 hunks)crates/consensus/src/hotstuff/on_leader_timeout.rs(1 hunks)crates/consensus/src/hotstuff/on_next_sync_view.rs(5 hunks)crates/consensus/src/hotstuff/on_propose.rs(1 hunks)crates/consensus/src/hotstuff/on_ready_to_vote_on_local_block.rs(1 hunks)crates/consensus/src/hotstuff/on_receive_local_proposal.rs(3 hunks)crates/consensus/src/hotstuff/on_receive_vote.rs(1 hunks)crates/consensus/src/hotstuff/pacemaker.rs(6 hunks)crates/consensus/src/hotstuff/pacemaker_handle.rs(3 hunks)crates/consensus/src/hotstuff/vote_collector/collector.rs(5 hunks)crates/consensus/src/hotstuff/vote_collector/proposal_collector.rs(1 hunks)crates/consensus/src/hotstuff/vote_collector/timeout_collector.rs(2 hunks)crates/consensus/src/hotstuff/worker.rs(20 hunks)crates/consensus/src/messages/catch_up.rs(1 hunks)crates/consensus/src/messages/message.rs(5 hunks)crates/consensus/src/messages/mod.rs(1 hunks)crates/consensus/src/traits/block_store.rs(1 hunks)crates/consensus/src/traits/certificate.rs(3 hunks)crates/consensus_tests/src/leader_failure.rs(6 hunks)crates/consensus_tests/src/substate_store.rs(1 hunks)crates/consensus_tests/src/support/harness.rs(3 hunks)crates/consensus_tests/src/support/network.rs(8 hunks)crates/consensus_tests/src/support/validator/builder.rs(1 hunks)crates/consensus_tests/src/support/validator/instance.rs(1 hunks)crates/consensus_types/src/bookkeeping/high_pc.rs(1 hunks)crates/consensus_types/src/bookkeeping/leaf_block.rs(1 hunks)crates/consensus_types/src/certificates/mod.rs(1 hunks)crates/consensus_types/src/certificates/quorum_certificate.rs(1 hunks)crates/p2p/src/conversions/consensus.rs(2 hunks)crates/state_store_rocksdb/src/column_families/diagnostic_no_vote.rs(1 hunks)crates/state_store_rocksdb/src/column_families/mod.rs(1 hunks)crates/state_store_rocksdb/src/options.rs(1 hunks)crates/state_store_rocksdb/src/reader.rs(4 hunks)crates/state_store_rocksdb/src/store.rs(2 hunks)crates/state_store_rocksdb/src/writer.rs(3 hunks)crates/storage/src/state_store/mod.rs(1 hunks)crates/template_builtin/templates/nft_faucet/src/lib.rs(1 hunks)crates/template_lib_types/src/crypto/ristretto.rs(1 hunks)crates/template_lib_types/src/crypto/scalar.rs(1 hunks)crates/template_lib_types/src/crypto/schnorr.rs(1 hunks)networking/core/src/worker.rs(1 hunks)utilities/db_inspector/src/webserver/server.rs(1 hunks)
💤 Files with no reviewable changes (1)
- applications/tari_indexer/web_ui/src/routes/VN/Components/NftRow.tsx
🧰 Additional context used
🧬 Code graph analysis (27)
crates/consensus/src/hotstuff/vote_collector/collector.rs (4)
crates/common_types/src/committee.rs (2)
quorum_threshold(65-67)quorum_threshold(253-255)crates/template_lib_types/src/crypto/ristretto.rs (1)
zero(27-29)crates/template_lib_types/src/crypto/scalar.rs (1)
zero(24-26)crates/template_lib_types/src/crypto/schnorr.rs (2)
zero(22-27)signature(64-66)
crates/storage/src/state_store/mod.rs (1)
crates/state_store_rocksdb/src/reader.rs (1)
parked_block_exists(1813-1817)
applications/tari_validator_node/src/bootstrap.rs (2)
crates/state_store_rocksdb/src/store.rs (1)
open(150-168)applications/tari_validator_node/src/config.rs (2)
default(142-171)default(190-194)
crates/template_lib_types/src/crypto/schnorr.rs (3)
crates/template_lib_types/src/crypto/ristretto.rs (1)
zero(27-29)crates/template_lib_types/src/crypto/scalar.rs (1)
zero(24-26)crates/template_lib_types/src/crypto/commitment_signature.rs (1)
zero(20-26)
crates/template_lib_types/src/crypto/scalar.rs (4)
crates/template_lib_types/src/crypto/ristretto.rs (1)
zero(27-29)crates/template_lib_types/src/crypto/schnorr.rs (1)
zero(22-27)crates/engine_types/src/hash.rs (1)
zero(29-31)crates/template_lib_types/src/crypto/commitment.rs (1)
zero(34-36)
crates/consensus/src/hotstuff/on_propose.rs (6)
crates/consensus/src/hotstuff/vote_collector/collector.rs (1)
height(285-287)crates/consensus_types/src/bookkeeping/high_pc.rs (1)
height(43-45)crates/consensus_types/src/bookkeeping/leaf_block.rs (1)
height(43-45)crates/consensus_types/src/certificates/quorum_certificate.rs (1)
height(40-45)crates/storage/src/consensus_models/block_header.rs (1)
height(387-389)crates/storage/src/consensus_models/block.rs (1)
height(348-350)
crates/consensus_tests/src/support/validator/builder.rs (2)
crates/state_store_rocksdb/src/store.rs (1)
open(150-168)crates/state_store_rocksdb/src/options.rs (1)
default(33-39)
crates/consensus_types/src/bookkeeping/high_pc.rs (4)
crates/consensus/src/hotstuff/vote_collector/collector.rs (1)
height(285-287)crates/consensus_types/src/bookkeeping/leaf_block.rs (1)
height(43-45)crates/consensus_types/src/certificates/quorum_certificate.rs (1)
height(40-45)bindings/src/types/NodeHeight.ts (1)
NodeHeight(3-3)
crates/consensus_tests/src/leader_failure.rs (2)
crates/consensus_tests/src/support/harness.rs (1)
network(362-364)crates/consensus_tests/src/support/network.rs (1)
total_messages_sent(166-168)
crates/consensus/src/hotstuff/vote_collector/proposal_collector.rs (2)
crates/consensus_types/src/bookkeeping/high_pc.rs (1)
block_id(39-41)crates/consensus_types/src/bookkeeping/leaf_block.rs (1)
block_id(47-49)
crates/consensus/src/hotstuff/block_change_set.rs (1)
crates/storage/src/consensus_models/transaction_pool.rs (1)
local_decision(420-422)
crates/consensus_types/src/certificates/quorum_certificate.rs (4)
bindings/src/types/Epoch.ts (1)
Epoch(3-3)bindings/src/types/NodeHeight.ts (1)
NodeHeight(3-3)bindings/src/types/ProposalCertificate.ts (1)
ProposalCertificate(7-15)bindings/src/types/TimeoutCertificate.ts (1)
TimeoutCertificate(6-13)
crates/consensus_tests/src/substate_store.rs (2)
crates/state_store_rocksdb/src/store.rs (1)
open(150-168)crates/state_store_rocksdb/src/options.rs (1)
default(33-39)
crates/consensus/src/hotstuff/on_catch_up_sync_request.rs (4)
crates/p2p/src/conversions/consensus.rs (15)
from(91-116)from(155-161)from(182-188)from(209-214)from(235-239)from(254-260)from(277-282)from(297-326)from(358-367)from(383-391)from(413-420)from(442-449)from(470-486)from(565-572)from(602-608)crates/consensus/src/messages/message.rs (1)
epoch(66-78)crates/consensus_types/src/bookkeeping/leaf_block.rs (2)
epoch(55-57)height(43-45)crates/storage/src/consensus_models/block.rs (1)
get_all_blocks_between(492-501)
crates/template_lib_types/src/crypto/ristretto.rs (3)
crates/template_lib_types/src/crypto/scalar.rs (1)
zero(24-26)crates/template_lib_types/src/crypto/schnorr.rs (1)
zero(22-27)crates/template_lib_types/src/crypto/commitment.rs (1)
zero(34-36)
crates/consensus/src/hotstuff/on_leader_timeout.rs (2)
crates/consensus/src/hotstuff/pacemaker.rs (1)
new(36-57)crates/consensus/src/hotstuff/pacemaker_handle.rs (1)
new(39-53)
crates/consensus/src/hotstuff/worker.rs (3)
crates/consensus/src/hotstuff/on_leader_timeout.rs (2)
default(52-54)wait(40-44)crates/consensus/src/hotstuff/common.rs (1)
get_highest_seen_justified_view(421-434)crates/consensus/src/hotstuff/on_inbound_message.rs (1)
next_message(35-57)
crates/consensus/src/hotstuff/on_ready_to_vote_on_local_block.rs (1)
crates/consensus/src/hotstuff/block_change_set.rs (1)
quorum_decision(306-308)
crates/consensus/src/hotstuff/on_receive_local_proposal.rs (2)
crates/consensus/src/traits/certificate.rs (3)
get(21-21)get(41-43)get(102-104)crates/storage/src/consensus_models/block.rs (1)
get(463-465)
crates/consensus/src/hotstuff/on_inbound_message.rs (4)
crates/consensus/src/messages/message.rs (1)
epoch(66-78)crates/consensus_types/src/bookkeeping/high_pc.rs (2)
epoch(51-53)height(43-45)crates/consensus_types/src/bookkeeping/leaf_block.rs (2)
epoch(55-57)height(43-45)crates/consensus_types/src/certificates/quorum_certificate.rs (2)
epoch(33-38)height(40-45)
crates/consensus/src/hotstuff/pacemaker.rs (2)
crates/consensus/src/hotstuff/worker.rs (1)
on_leader_timeout(648-679)crates/consensus/src/hotstuff/on_leader_timeout.rs (1)
leader_timed_out(46-48)
crates/consensus/src/hotstuff/on_next_sync_view.rs (2)
bindings/src/types/Epoch.ts (1)
Epoch(3-3)bindings/src/types/NodeHeight.ts (1)
NodeHeight(3-3)
crates/consensus/src/traits/certificate.rs (4)
crates/consensus/src/hotstuff/vote_collector/collector.rs (1)
height(285-287)crates/consensus_types/src/bookkeeping/high_pc.rs (1)
height(43-45)crates/consensus_types/src/bookkeeping/leaf_block.rs (1)
height(43-45)crates/consensus_types/src/certificates/quorum_certificate.rs (1)
height(40-45)
crates/state_store_rocksdb/src/reader.rs (2)
crates/storage/src/state_store/mod.rs (1)
parked_block_exists(328-328)crates/state_store_rocksdb/src/cf_api.rs (1)
exists(130-137)
crates/consensus/src/hotstuff/on_catch_up_sync.rs (1)
crates/consensus/src/messages/message.rs (1)
epoch(66-78)
crates/state_store_rocksdb/src/writer.rs (1)
crates/state_store_rocksdb/src/cf_api.rs (1)
cf(32-41)
crates/state_store_rocksdb/src/store.rs (1)
crates/state_store_rocksdb/src/column_families/diagnostic_no_vote.rs (1)
name(25-27)
🔇 Additional comments (72)
crates/consensus/src/traits/block_store.rs (1)
80-80: Helpful clarification of the 3-chain invariant.The comment accurately documents the parent-child relationship being validated in the conditional on line 81. This improves code clarity for readers unfamiliar with the consensus protocol's 3-chain locking mechanism.
applications/tari_validator_node/src/p2p/rpc/state_sync_task.rs (1)
79-79: Verify the log level downgrade for sync completion.This changes the state sync completion message from info to debug level. Since sync completion is an operational milestone, info-level logging is typically more appropriate for production monitoring.
Additionally, this change isn't mentioned in the PR description, which focuses on consensus catch-up sync rather than state sync.
Please confirm:
- Is this log level change intentional?
- Should sync completion messages be visible in default (info-level) logs for operational visibility?
crates/consensus_tests/src/support/validator/instance.rs (1)
98-103: Excellent defensive improvement for test robustness.The refactoring from direct
unwrap()to.optional().unwrap()with let-else guards prevents panics when transaction records or finalized executions are missing. Returningfalseearly is semantically correct—if the data doesn't exist, the transaction hasn't committed substates. This aligns well with the PR's goal of making catch-up synchronization more robust.crates/consensus/src/hotstuff/vote_collector/proposal_collector.rs (1)
71-76: LGTM! Good diagnostic addition.This debug log enhances observability by confirming successful vote validation before collection begins. The placement is appropriate, the log level is correct, and it aligns with the PR's goal of improving diagnostics.
applications/tari_indexer/web_ui/src/routes/VN/Components/Resources.tsx (1)
104-104: Flag: Changes unrelated to PR objectives.The PR objectives describe consensus catch-up synchronization fixes, but this file contains UI changes to remove the
original_ownerfield from NFT data processing. This suggests either:
- The PR contains unrelated changes that should be in a separate PR, or
- The PR objectives are incorrect or incomplete.
Please clarify whether these NFT UI changes are intentionally included in this consensus-focused PR.
applications/tari_walletd/web_ui/src/routes/AssetVault/NFTs/components/NftParts.tsx (1)
111-121: Flag: Changes unrelated to PR objectives.Similar to the Resources.tsx file, this NFT metadata rendering change appears unrelated to the consensus catch-up synchronization fixes described in the PR objectives. The replacement of
original_ownerdisplay with generic metadata extraction seems to be part of a different feature set.crates/template_builtin/templates/nft_faucet/src/lib.rs (1)
46-46: No code references to "original_owner" found—migration is complete.Comprehensive search across the entire codebase found zero references to the old
"original_owner"metadata key. The code at line 46 is already using the new"original_minter"key, and no other code queries, filters, or references to the deprecated key exist.crates/consensus_types/src/certificates/mod.rs (1)
6-6: LGTM! Module addition follows existing patterns.The new
quorum_certificatemodule and its re-export are consistent with the structure used for other certificate types in this module.Also applies to: 12-12
crates/consensus_types/src/certificates/quorum_certificate.rs (5)
1-8: LGTM! Clean imports and dependencies.The imports are appropriate for the enum's serialization requirements and delegate to the inner certificate types.
10-15: LGTM! Well-structured enum definition.The derives are appropriate for consensus data structures, and the conditional TypeScript export support enables cross-language type safety.
18-24: LGTM! Idiomatic variant checks.Using
matches!for boolean variant checks is the standard Rust pattern.
26-31: LGTM! Useful debugging helper.Returning static strings for variant names is efficient and useful for logging and diagnostics.
33-45: LGTM! Clean delegation to inner certificates.The delegation of
epoch()andheight()provides a unified interface while correctly routing to the appropriate inner certificate implementation.crates/template_lib_types/src/crypto/schnorr.rs (1)
22-27: ConstSchnorrSignatureBytes::zero()looks correct and const-safe
zero()now only calls other const fns and constructs a struct literal, so making itpub const fnis sound and preserves behavior while enabling const-context use. Just ensure your MSRV/CI toolchain supports this const usage pattern.crates/template_lib_types/src/crypto/ristretto.rs (1)
27-29: ConstRistrettoPublicKeyBytes::zero()is consistent and non-breakingThe move to
pub const fnis compatible with the existing body (array literal with const length) and simply broadens usage to const contexts; runtime semantics stay the same.crates/template_lib_types/src/crypto/scalar.rs (1)
24-26: ConstScalar32Bytes::zero()aligns with other crypto zero-constructorsThis change is purely to const-qualify an already simple zero-initializer; it improves const usability without altering behavior and keeps the API consistent with
RistrettoPublicKeyBytes::zero().crates/consensus_tests/src/support/harness.rs (1)
249-281: Address-based offline filtering inon_block_committedlooks soundFiltering Hotstuff events with
self.network.is_offline(&address).awaitbefore matching avoids spurious failures from deliberately offline nodes and aligns with the new address-based offline API. This should make leader-failure tests more robust without affecting normal runs.crates/consensus_tests/src/support/validator/builder.rs (1)
142-149: Enabling RocksDB debugging data for tests aligns with diagnostics goalsOpening
TestStorewithDatabaseOptions::default().with_debugging_data(true)is a good fit for consensus tests: it enables the new diagnostics/no-vote data without affecting production code paths. Given that test stores are created in temp or cleared directories, the extra data volume is unlikely to be an issue.crates/state_store_rocksdb/src/column_families/mod.rs (1)
34-37: New diagnostics column-family module export is straightforwardAdding
pub mod diagnostic_no_vote;cleanly exposes the new diagnostics/no-vote CF alongside the existing CF modules and matches the rest of the module layout.crates/consensus_tests/src/substate_store.rs (1)
233-233: LGTM!Enabling debugging data in test environments is appropriate and will help with diagnosing test failures.
crates/state_store_rocksdb/src/store.rs (1)
41-41: LGTM!The new
DiagnosticsNoVoteCfcolumn family is properly imported and registered in the list of all column families, following the established pattern.Also applies to: 127-127
crates/storage/src/state_store/mod.rs (1)
327-328: LGTM!The new
parked_block_existsmethod is a sensible addition to the read transaction API, allowing existence checks without fetching the full parked block data.utilities/db_inspector/src/webserver/server.rs (1)
113-114: LGTM!The new
DiagnosticsNoVoteCfis properly exposed through the web UI, following the established pattern for other column families.crates/state_store_rocksdb/src/options.rs (1)
18-29: LGTM!The
debugging_dataoption is well-designed:
- Defaults to
falseto avoid unintended performance impact- Includes clear documentation about the trade-offs
- Follows the builder pattern consistently with other options
Also applies to: 37-37
crates/state_store_rocksdb/src/column_families/diagnostic_no_vote.rs (1)
1-28: LGTM!The diagnostic column family is well-designed:
- Uses
Box<str>for the reason field, which is more memory-efficient thanStringfor immutable data- Follows the established
Cftrait pattern consistently- Keyed by
BlockId, which makes sense for tracking no-vote decisions per blockcrates/state_store_rocksdb/src/writer.rs (3)
120-120: LGTM!Import of the new diagnostic column family types is correct.
859-868: LGTM!The conditional write to the debug history CF is properly gated by
self.options.debugging_data, ensuring no performance impact when debugging is disabled.
1726-1739: LGTM!The
diagnostics_add_no_voteimplementation is well-designed:
- Properly gated by
self.options.debugging_datato avoid performance impact when disabled- Uses
insertoperation which is appropriate for diagnostic data- Converts
NoVoteReasonto string only when needed- Follows the established error handling pattern with the OPERATION constant
crates/state_store_rocksdb/src/reader.rs (3)
134-134: LGTM: Import addition supports new parked block functionality.The parked_block import is correctly added to support the new
parked_block_existsmethod implementation below.
1813-1817: LGTM: Correct implementation of parked block existence check.The method correctly implements the trait interface for checking parked block existence, following the same pattern as other existence checks in this file (e.g.,
blocks_exists,foreign_parked_blocks_exists). This aligns with the PR objective of making catch-up sync more robust by enabling better handling of parked blocks.
735-739: No breaking changes from the limit reduction—verified safe.The limit reduction from 1,000,000 to 1,000 does not break existing callers. The only active call site (
crates/consensus/src/hotstuff/on_catch_up_sync_request.rs:111) useslimit=100, which is well below the new threshold. All test code also uses limits ≤ 10. The change is defensive and safe.crates/consensus/src/hotstuff/on_propose.rs (1)
344-344: LGTM! Enhanced diagnostic logging.The addition of justify and highest_seen_block heights to the debug log improves observability during block proposal, which directly supports the PR's goal of making catch-up synchronization more robust.
networking/core/src/worker.rs (1)
69-69: LGTM! Logging namespace cleanup.The LOG_TARGET simplification removes "ootle" from the namespace path, aligning with the broader logging ecosystem reorganization mentioned in the PR context.
applications/tari_validator_node/src/p2p/rpc/rpc_impl.rs (1)
405-405: LGTM! Appropriate log level adjustment.Downgrading the peer-initiated sync message from info to debug level is appropriate, as state synchronization operations may be frequent and don't require info-level visibility in production logs.
crates/consensus/src/traits/certificate.rs (2)
64-64: LGTM! API consistency improvement.The change from
block_height()toheight()aligns the HighPc API with other consensus types (LeafBlock, QuorumCertificate, TimeoutCertificate) that also expose aheight()method, improving consistency across the codebase.
74-74: LGTM! Logging updated for API consistency.The log messages correctly reflect the API change from
block_height()toheight(), maintaining consistency throughout the method.Also applies to: 85-85
applications/tari_validator_node/src/p2p/rpc/mod.rs (1)
24-27: LGTM! Internal module refactoring.The module rename from
service_impltorpc_implis an internal refactoring that improves naming clarity while preserving the public API surface through the re-export ofValidatorNodeRpcServiceImpl.crates/consensus/src/hotstuff/on_next_sync_view.rs (3)
32-33: LGTM! In-memory cache for performance.Adding
last_sent_new_viewas an in-memory cache improves performance by avoiding repeated storage reads during NEWVIEW message processing, which directly supports the PR's goal of more robust catch-up synchronization.
47-47: LGTM! Proper field initialization.Initializing
last_sent_new_viewtoNoneis correct, representing the initial state where no NEWVIEW message has been sent yet.
120-127: LGTM! Proper cache and storage synchronization.The dual update pattern correctly maintains both the in-memory cache (line 120) and persistent storage (lines 121-127), ensuring fast reads while preserving data across restarts.
crates/consensus/src/hotstuff/vote_collector/timeout_collector.rs (2)
16-16: LGTM! Logging namespace alignment.The LOG_TARGET update adds "ootle" to the namespace path, aligning with the consensus module's logging organization mentioned in the PR context.
78-78: LGTM! Enhanced timeout vote observability.The debug log when no quorum is reached provides valuable diagnostic information for troubleshooting timeout vote collection issues, which aligns with the PR's goal of more robust catch-up synchronization.
crates/consensus/src/hotstuff/common.rs (1)
427-427: LGTM! Consistent API usage.The change from
block_height()toheight()maintains API consistency with other files in this PR and aligns with the HighPc type's updated interface.crates/consensus_types/src/bookkeeping/leaf_block.rs (1)
39-41: LGTM! Clean semantic helper for genesis detection.The
is_genesis()method provides a clear and idiomatic way to check if a block is the genesis block, improving code readability.crates/consensus/src/messages/mod.rs (1)
24-26: LGTM! Module renamed for clarity.The renaming from
synctocatch_upbetter reflects the specific synchronization mechanism and aligns with the PR's goal to make catch-up synchronization more robust.crates/consensus/src/messages/catch_up.rs (1)
8-11: LGTM! Clear naming improvement.The rename from
SyncRequestMessagetoCatchUpRequestMessageimproves clarity and aligns with the PR's catch-up synchronization improvements.crates/consensus/src/hotstuff/error.rs (1)
170-175: LGTM! New error variant for parked justify block handling.This error variant is essential for the PR's fix to handle scenarios where the justify block is parked during catch-up. The error message provides clear context with the proposer, block description, and the parked justify block.
crates/consensus/src/hotstuff/vote_collector/collector.rs (3)
17-17: LGTM! Logging target updated for consistency.The logging target change from
tari::consensus::hotstuff::vote_collectortotari::ootle::consensus::hotstuff::vote_collectoraligns with the broader logging restructuring in the PR.
59-91: LGTM! Appropriate logging level adjustment.Reducing the verbosity of vote collection logs from
info!todebug!is a sensible change, as these messages are primarily useful for debugging rather than general operational visibility.
239-325: LGTM! Good test coverage for vote storage.The new tests cover the essential behavior of
VoteStoreInner:
it_saves_a_new_vote: Validates successful vote insertionit_detects_a_duplicate_vote: Ensures duplicate votes are properly rejectedThe
TestVoteimplementation is minimal but sufficient for these unit tests.crates/consensus/src/hotstuff/on_receive_vote.rs (1)
58-61: LGTM! Variable and API usage updated correctly.The rename from
high_qctohigh_pcand the updated call toreset_leader_timeout(&high_pc)correctly align with theHighPcAPI changes elsewhere in the PR.applications/tari_validator_node/log4rs_sample.yml (1)
154-182: LGTM! Logging configuration updated to match code structure.The logging configuration changes align with the PR's restructuring:
- Commented out direct
ootleappender for the maintari::ootlelogger- Added explicit loggers for
tari::consensusandtari::ootle::storage- Renamed
tari::ootle::networkingtotari::ootle::hotstufffor clarity- Added
tari::networkingloggerAll new loggers route to the
consensusappender with debug level, providing better log separation and organization.crates/consensus_types/src/bookkeeping/high_pc.rs (1)
43-45: All call sites correctly updated to useheight()method.Verification confirms the method rename from
block_height()toheight()on theHighPctype is complete. No remaining calls to.block_height()onHighPcinstances were found, and the newheight()method is actively used throughout the codebase (e.g.,high_pc.height()inon_next_sync_view.rs). Other types likeTransactionExecutionandLastSentVoteretain their ownblock_height()methods, which remains correct and unchanged.crates/consensus/src/hotstuff/block_change_set.rs (2)
58-58: LGTM: Field rename improves semantic clarity.The rename from
quorum_decisiontolocal_decisionbetter reflects that this represents the local node's decision about a block, not necessarily a quorum decision. The change is consistently applied throughout the file.Also applies to: 69-69
93-93: API change has been successfully applied across all active code.The HighPc type now exposes
.height()as the public method (internally wrapping theblock_heightfield), and no publicblock_height()method exists. All active callsites consistently use the new.height()API. The one ripgrep match in manager_old.rs is in dead code—the module is commented out and not part of the active codebase.crates/consensus/src/hotstuff/on_catch_up_sync.rs (1)
11-11: LGTM: Message type rename is consistent.The rename from
SyncRequestMessagetoCatchUpRequestMessageis consistently applied in imports and usage. The new name is more descriptive of the message's purpose.Also applies to: 63-63
crates/consensus/src/hotstuff/on_ready_to_vote_on_local_block.rs (1)
198-205: LGTM: BlockDecision construction updated with new field.The addition of
new_high_tctoBlockDecisionaligns with the PR's changes to track high timeout certificates. The field rename tolocal_decisionis consistent with changes inblock_change_set.rs.crates/consensus/src/hotstuff/on_receive_local_proposal.rs (1)
373-373: LGTM: Consistent with field and method renames.The changes to use
local_decisionandhigh_pc.height()are consistent with the broader refactoring across this PR.Also applies to: 379-379
crates/consensus/src/hotstuff/pacemaker_handle.rs (2)
126-127: Documentation improvement noted.The updated documentation clarifies that providing a view earlier than the current view results in a no-op. This is helpful for understanding the behavior.
103-114: No issues found - API change properly implemented.The search confirms that all callsites have been updated correctly. The only callsite at
on_receive_vote.rs:61already passes&high_pcas required by the new signature.crates/p2p/src/conversions/consensus.rs (1)
30-30: LGTM: Proto conversion layer updated consistently.The conversion implementations correctly renamed
SyncRequestMessagetoCatchUpRequestMessagewhile maintaining the same proto message type (proto::consensus::SyncRequest). This maintains wire compatibility while improving internal naming.Also applies to: 1037-1055
crates/consensus/src/hotstuff/pacemaker.rs (1)
88-88: LGTM: Timeout counter provides valuable context.The addition of
num_timeoutstracking and theLeaderTimeoutstruct enriches the timeout notification with context including:
current_height: The view height when timeout occurredcurrent_high_pc: The high QC heightnum_timeouts: Count of consecutive timeoutsThis enables downstream logic to trigger catch-up sync after multiple consecutive timeouts (as noted in the related
worker.rssnippet showing a check fort.num_timeouts.is_multiple_of(3)), improving the robustness of the consensus protocol.Also applies to: 103-103, 157-162, 186-191
crates/consensus/src/hotstuff/on_catch_up_sync_request.rs (2)
36-46: LGTM: Updated to use CatchUpRequestMessage consistently.The parameter type change and early return logic for non-matching epochs and zero-height nodes are appropriate guards.
Also applies to: 52-106
108-177: Code changes are sound; O(n) foreign proposal queries noted as optimization opportunity but not a blocking issue.The batched catch-up implementation is correctly implemented:
- Blocks fetched in batches of 100 (line 112)
- Foreign proposals retrieved per block (line 148)
- All errors properly logged before early return (lines 117-120, 130-131, 151-154, 173-175)
The TODO comment on line 148 correctly identifies that
block.get_foreign_proposals(tx)results in O(n) database queries within each 100-block batch. The storage layer already supports batch fetching viaForeignProposalRecord::get_any(), but optimizing this would require restructuring the loop to collect all block IDs first, then mapping results—a reasonable follow-up enhancement rather than a blocker for this change.Error handling is comprehensive: all failure paths log warnings and return gracefully.
crates/consensus/src/hotstuff/worker.rs (4)
314-315: Startup and timeout-triggered catch-up requestsUnconditionally issuing a
request_catch_up_syncat the start ofrunand then reusing the same helper on every 3rd leader timeout (when notis_catching_up()) is a nice way to gently probe for missing state without fully entering the “catch up” mode.Because
request_catch_up_syncdoes not mutateworker_state.catch_up, it correctly avoids interacting with the stricter “catching up” gating inpropose_nowandon_leader_timeout. Behavior looks consistent and low‑risk; the early return when there’s no other committee member is also reasonable.Also applies to: 603-616, 667-675
388-390: Leader timeout payload and catch-up triggerThreading
LeaderTimeoutintoon_leader_timeoutand usingnum_timeouts.is_multiple_of(3)as a threshold before requesting catch up, guarded by!worker_state.is_catching_up(), looks sound. It avoids spamming catch‑up sync while still reacting to sustained leader failures, and the log fields (timeout: {}) make debugging easier.Given this is consensus‑critical, please make sure any other call sites that invoke
on_leader_timeoutwithNone(e.g. NOVOTE paths) are not expected to incrementnum_timeoutsin the pacemaker; right now those calls deliberately skip the “3 failures” catch‑up trigger, which seems intentional.Also applies to: 648-679
1015-1063: Catch-up batching and first-block state inon_proposal_messageLinking proposal handling to the new
CatchUpstate andhas_processed_first_blockis a good way to coordinate catch-up progress with the worker:
- The
catch_upprogress updates on every proposal, advancing batches viaset_next_batchand logging whether we’re still catching up or done; after the final batch,catch_upremainsNone, which correctly exits “catching up” mode.- Marking
has_processed_first_block = trueforNone | Some(NoVoteReason::AlreadyVotedAtHeight)gives a reasonable approximation of “we’re actually processing height‑1” while still treating other NOVOTE reasons as leader failures that should immediately trigger NEWVIEW.The behavior looks consistent with the intent of the PR; just keep in mind that any new
NoVoteReasonvariants added in future that still imply “first block is effectively processed” should be explicitly whitelisted in this match.
1235-1257: NewWorkerStateandCatchUpstructsThe small
WorkerState+CatchUpstructs give a clear place to track catch-up progress and first-block processing state, and theis_catching_uphelper keeps the intent obvious at call sites. TheCatchUp::set_next_batchlogic (with clamping tohigh_qc) is a good fit for fixed-size catch-up batches once the initialexpected_batch_heightis corrected as above.crates/consensus/src/hotstuff/on_leader_timeout.rs (1)
9-22: RicherLeaderTimeoutpayload andOnLeaderTimeoutAPISwitching the watch channel payload from
NodeHeightto the newLeaderTimeoutstruct looks clean: it preserves the previous semantics (waitstill waits for a change and returns the latest value) while adding useful context (current_high_pc,num_timeouts, and a convenientdelta()).The
leader_timed_outAPI remains simple, andDefaultfor bothLeaderTimeoutandOnLeaderTimeoutkeeps initialization straightforward. This integrates well with the worker changes that log and act onnum_timeouts.Also applies to: 24-48
crates/consensus/src/messages/message.rs (1)
18-41: Catch-up message variant andinto_proposalhelperThe new
CatchUpSyncRequest(CatchUpRequestMessage)variant is wired consistently throughHotstuffMessage:
as_type_strandepoch()both handle the new variant, so logging and epoch-based routing continue to work.Dispatchand inbound handling (in other files) now see a dedicated catch-up message type.The
into_proposal(self) -> Option<ProposalMessage>helper is a nice complement toproposal(&self), letting call sites take ownership without additional branching or cloning.The
Displayimplementation usingSelf::Variantpatterns reads cleanly; keeping the catch-up string label as"SyncRequest({}/{})"maintains log continuity even though the enum variant is nowCatchUpSyncRequest.Also applies to: 52-77, 80-93, 95-147
crates/consensus/src/hotstuff/on_inbound_message.rs (2)
35-45: Threadinghas_processed_first_blockinto inbound bufferingPassing
has_processed_first_blockdown intoMessageBuffer::nextand gating buffer draining on that flag is a good way to avoid processing higher‑view proposals before the first block has successfully gone through:
split_off(&(current_epoch, next_height))correctly drops buffered messages for older epochs/heights.- When
has_processed_first_blockisfalse, you avoid draining the(epoch, next_height)bucket, so “future” proposals stay buffered until the first block is actually processed.- Once the flag flips to
true, buffered current‑view messages can be drained as before.This matches the described bug scenario and should prevent the catch-up → error → catch-up loop when the first block is parked.
Also applies to: 83-103
363-368: RestrictingCatchUpSyncRequestto current epochTreating
CatchUpSyncRequestasDiscardwhenmsg.epoch != current_epochprovides a clear, fail‑fast rule: we only honor catch-up requests for the epoch we’re currently running consensus in. That avoids mixing state across epochs at the inbound routing layer and seems consistent with how epoch-specific catch-up is handled elsewhere.
Test Results (CI)518 tests +3 499 ✅ - 6 1h 25m 0s ⏱️ - 4m 26s For more details on these failures, see this check. Results for commit d49c3b9. ± Comparison against base commit 3b86ab7. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/template_builtin/templates/faucet/src/lib.rs (1)
35-40: Consistent validation applied to confidential withdrawals.The same maximum withdrawal limit is appropriately enforced for confidential transfers, maintaining consistency with the
takemethod.
🧹 Nitpick comments (2)
crates/template_builtin/templates/faucet/src/lib.rs (2)
8-8: Inconsistent underscore placement in numeric literal.The numeric literal
1000_000_000uses underscores inconsistently. Rust convention is to group digits in thousands using underscores consistently (e.g.,1_000_000_000).Apply this diff to improve readability:
- const FAUCET_MAX: u64 = 1000_000_000; + const FAUCET_MAX: u64 = 1_000_000_000;
17-22: Good addition of faucet withdrawal limit.The assertion correctly enforces a maximum withdrawal amount with a clear error message. This is an appropriate security measure for a faucet.
Note: The same assertion logic is duplicated in
take_confidential(lines 35-40). While acceptable for such a simple check, consider extracting to a helper method if more validation logic is added in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
applications/tari_walletd/web_ui/src/routes/AssetVault/NFTs/components/NftParts.tsx(2 hunks)crates/template_builtin/templates/faucet/src/lib.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- applications/tari_walletd/web_ui/src/routes/AssetVault/NFTs/components/NftParts.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-11T07:43:03.991Z
Learnt from: sdbondi
Repo: tari-project/tari-ootle PR: 1639
File: applications/tari_app_utilities/src/shared_consts.rs:6-7
Timestamp: 2025-11-11T07:43:03.991Z
Learning: In the Tari codebase, XTR uses micro-units where 1 XTR = 1,000,000 micro XTR. When reviewing Amount values, u64::MAX in micro XTR equals approximately 18.45 trillion whole XTR, not quintillion.
Applied to files:
crates/template_builtin/templates/faucet/src/lib.rs
🧬 Code graph analysis (1)
crates/template_builtin/templates/faucet/src/lib.rs (2)
crates/engine_types/src/proof.rs (2)
amount(42-44)amount(84-86)crates/template_lib/src/models/bucket.rs (1)
amount(206-214)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test
- GitHub Check: clippy
- GitHub Check: check stable
- GitHub Check: check nightly
- GitHub Check: machete
d49c3b9 to
67b43a1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
crates/consensus/src/hotstuff/on_receive_local_proposal.rs (1)
728-730: Critical:JustifyBlockParkederror not handled in match.The past review comment remains unresolved. The error matching at lines 728-730 only explicitly handles
JustifyBlockNotFound, but the new code at line 819 can also returnJustifyBlockParked. When this occurs, it falls through to the genericProposalValidationErrorhandler at line 732, bypassing the intended catch-up sync path and breaking the core fix described in the PR objectives.Update the match to handle both
JustifyBlockNotFoundandJustifyBlockParked:-Err(err @ HotStuffError::ProposalValidationError(ProposalValidationError::JustifyBlockNotFound { .. })) => { +Err(err @ HotStuffError::ProposalValidationError( + ProposalValidationError::JustifyBlockNotFound { .. } | + ProposalValidationError::JustifyBlockParked { .. } +)) => { Err(err) },crates/consensus/src/hotstuff/on_next_sync_view.rs (1)
64-68: Critical: Bidirectional cache invalidation still missing.The past review comment remains unresolved. When
storage.last_sent_new_view_clear()is called (e.g., atcertificate.rs:68oron_receive_new_view.rs:164), the persistent storage is cleared butself.last_sent_new_viewis not. This causes stale same-epoch data to be reused, potentially causing the timeout height to skip unnecessarily.Add bidirectional invalidation: whenever storage is cleared, also set
self.last_sent_new_view = None, or implement a helper that clears both cache levels atomically.
🧹 Nitpick comments (10)
crates/state_store_rocksdb/src/column_families/diagnostic_no_vote.rs (2)
12-15: Add documentation for the public DiagnosticsNoVoteData struct.This struct is part of the new public API for diagnostic no-vote tracking. Consider adding doc comments to explain:
- What diagnostic data this struct represents
- When/why a no-vote occurs
- The semantics of the
reasonfieldExample:
+/// Diagnostic data captured when a validator chooses not to vote on a block. +/// +/// This data is persisted to aid in debugging consensus behavior and is only +/// stored when the `debugging_data` option is enabled. #[derive(Debug, Serialize, Deserialize, Clone)] pub struct DiagnosticsNoVoteData { + /// Human-readable reason explaining why no vote was cast for this block. pub reason: Box<str>, }
17-28: Add documentation for the DiagnosticsNoVoteCf column family.Document the purpose and usage of this column family to help developers understand when and how diagnostic no-vote data is stored.
Example:
+/// Column family for storing diagnostic data about blocks that did not receive votes. +/// +/// This CF maps block IDs to diagnostic information explaining why a validator +/// chose not to vote. Data is only persisted when the `debugging_data` configuration +/// option is enabled. pub struct DiagnosticsNoVoteCf;crates/consensus/src/hotstuff/vote_collector/collector.rs (2)
84-91: Consider retaining info-level for quorum-reached events.Reaching quorum is a significant consensus milestone. While reducing verbosity is valuable, this particular event may warrant info-level logging for easier production troubleshooting and monitoring.
239-325: LGTM: Good foundational test coverage.The new test module appropriately validates basic vote storage and duplicate detection. The TestVote implementation with zero constants is acceptable for unit tests.
For more comprehensive coverage, consider adding tests for:
calculate_threshold_decisionwith mixed Accept/Reject votes- Quorum threshold boundary conditions
clear_votes_beforecleanup behaviorApply this diff to add a test for mixed decision quorum calculation:
#[test] fn it_calculates_threshold_decision_with_mixed_votes() { use tari_ootle_common_types::ValidatorPublicKey; let store = VoteStoreInner::<TestVote>::new(); let vote = TestVote { epoch: Epoch(1), height: NodeHeight(1), key: 42, }; // TODO: Create a committee with known validators and voting power // TODO: Save votes with different decisions (Accept/Reject) // TODO: Assert correct threshold calculation based on committee power }crates/consensus/src/hotstuff/on_catch_up_sync_request.rs (1)
108-176: Robust iterative catch-up logic, but note O(n) query concern.The refactored logic now processes catch-up requests iteratively, fetching and sending blocks in chunks of up to 100. This is more robust for large height differences and aligns with the PR's goal of making catch-up more resilient.
However, the TODO comment on line 147 correctly notes the O(n) query concern for fetching foreign proposals per block. For large catch-up ranges, this could result in many database queries. Consider batching foreign proposal fetches in a follow-up optimization if catch-up performance becomes an issue.
crates/consensus/src/hotstuff/worker.rs (4)
667-667: Verify the multiple-of-3 timeout threshold is appropriate.The code triggers catch-up sync after every 3rd consecutive leader timeout. While this prevents excessive catch-up requests, it might delay synchronization if the node is genuinely behind. Consider whether this threshold aligns with expected network conditions and whether it should be configurable.
848-861: Clarify the logic preventing proposals after voting.The check prevents proposing if
last_sent_vote.block_height() >= next_height. While this is correct for preventing duplicate proposals during catch-up, the relationship between voting and proposing could be clearer. Consider adding a comment explaining why sending a vote at a height means we shouldn't propose at that height (e.g., "We've already participated in consensus at this height by voting, so we shouldn't propose").
1253-1257: Verify catch-up batch boundary logic.The
set_next_batchmethod calculates the next expected batch height as(current_height + 99).min(high_qc)and returns true ifexpected_batch_height < high_qc. This logic assumes we process exactly up to the expected batch height before calling this method again.Consider adding a comment explaining:
- Why 99 blocks per batch (rather than 100)
- Whether the boundary condition correctly handles the case where we reach exactly
high_qc- How this interacts with the message buffering logic
250-255: Initialization logic forhas_processed_first_blockis flawed and should be corrected.The flag is set to
!is_genesis(line 255) beforecurrent_heightis retrieved from persistent state (lines 258-260). This creates an inconsistency: if the node crashes immediately after creating genesis, the next restart will sethas_processed_first_block = true(because genesis now exists) butcurrent_height = 0(no justified votes recorded).The flag should instead be derived from the actual justified height:
has_processed_first_block = (current_height > 0). This would correctly reflect whether any consensus progress has been persisted. While a workaround exists inon_inbound_message.rs(line 226-232, marked "TODO: hacky"), fixing the root cause at initialization is cleaner and more maintainable.Recommended: Retrieve
current_heightfirst, then initialize the flag based on its value.crates/consensus/src/hotstuff/on_inbound_message.rs (1)
212-244: Complex first-block handling logic could benefit from refactoring.The conditional logic based on
has_processed_first_blockcorrectly addresses the catch-up loop issue described in the PR objectives. However, the special case handling (lines 226-237) is marked as "TODO: hacky" by the developers, indicating it's a pragmatic fix rather than a clean solution.While this implementation is functional, consider whether the genesis/startup case could be handled more cleanly in a future refactor, perhaps by explicitly tracking "genesis startup mode" as a distinct state from normal operation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (58)
applications/tari_indexer/web_ui/src/routes/VN/Components/NftRow.tsx(0 hunks)applications/tari_indexer/web_ui/src/routes/VN/Components/Resources.tsx(1 hunks)applications/tari_validator_node/log4rs_sample.yml(2 hunks)applications/tari_validator_node/src/bootstrap.rs(1 hunks)applications/tari_validator_node/src/p2p/rpc/mod.rs(1 hunks)applications/tari_validator_node/src/p2p/rpc/rpc_impl.rs(1 hunks)applications/tari_validator_node/src/p2p/rpc/state_sync_task.rs(1 hunks)applications/tari_walletd/web_ui/src/routes/AssetVault/NFTs/components/NftParts.tsx(2 hunks)crates/consensus/src/hotstuff/block_change_set.rs(3 hunks)crates/consensus/src/hotstuff/common.rs(1 hunks)crates/consensus/src/hotstuff/error.rs(1 hunks)crates/consensus/src/hotstuff/on_catch_up_sync.rs(3 hunks)crates/consensus/src/hotstuff/on_catch_up_sync_request.rs(5 hunks)crates/consensus/src/hotstuff/on_inbound_message.rs(5 hunks)crates/consensus/src/hotstuff/on_leader_timeout.rs(1 hunks)crates/consensus/src/hotstuff/on_next_sync_view.rs(5 hunks)crates/consensus/src/hotstuff/on_propose.rs(1 hunks)crates/consensus/src/hotstuff/on_ready_to_vote_on_local_block.rs(11 hunks)crates/consensus/src/hotstuff/on_receive_local_proposal.rs(3 hunks)crates/consensus/src/hotstuff/on_receive_vote.rs(1 hunks)crates/consensus/src/hotstuff/pacemaker.rs(6 hunks)crates/consensus/src/hotstuff/pacemaker_handle.rs(3 hunks)crates/consensus/src/hotstuff/vote_collector/collector.rs(5 hunks)crates/consensus/src/hotstuff/vote_collector/proposal_collector.rs(1 hunks)crates/consensus/src/hotstuff/vote_collector/timeout_collector.rs(2 hunks)crates/consensus/src/hotstuff/worker.rs(20 hunks)crates/consensus/src/messages/catch_up.rs(1 hunks)crates/consensus/src/messages/message.rs(5 hunks)crates/consensus/src/messages/mod.rs(1 hunks)crates/consensus/src/traits/block_store.rs(1 hunks)crates/consensus/src/traits/certificate.rs(3 hunks)crates/consensus_tests/src/leader_failure.rs(6 hunks)crates/consensus_tests/src/substate_store.rs(1 hunks)crates/consensus_tests/src/support/harness.rs(3 hunks)crates/consensus_tests/src/support/network.rs(8 hunks)crates/consensus_tests/src/support/validator/builder.rs(1 hunks)crates/consensus_tests/src/support/validator/instance.rs(1 hunks)crates/consensus_types/src/bookkeeping/high_pc.rs(1 hunks)crates/consensus_types/src/bookkeeping/leaf_block.rs(1 hunks)crates/consensus_types/src/certificates/mod.rs(1 hunks)crates/consensus_types/src/certificates/quorum_certificate.rs(1 hunks)crates/p2p/src/conversions/consensus.rs(2 hunks)crates/state_store_rocksdb/src/column_families/diagnostic_no_vote.rs(1 hunks)crates/state_store_rocksdb/src/column_families/mod.rs(1 hunks)crates/state_store_rocksdb/src/options.rs(1 hunks)crates/state_store_rocksdb/src/reader.rs(4 hunks)crates/state_store_rocksdb/src/store.rs(2 hunks)crates/state_store_rocksdb/src/writer.rs(3 hunks)crates/storage/src/consensus_models/block_header.rs(1 hunks)crates/storage/src/consensus_models/no_vote.rs(2 hunks)crates/storage/src/state_store/mod.rs(1 hunks)crates/template_builtin/templates/faucet/src/lib.rs(3 hunks)crates/template_builtin/templates/nft_faucet/src/lib.rs(1 hunks)crates/template_lib_types/src/crypto/ristretto.rs(1 hunks)crates/template_lib_types/src/crypto/scalar.rs(1 hunks)crates/template_lib_types/src/crypto/schnorr.rs(1 hunks)networking/core/src/worker.rs(1 hunks)utilities/db_inspector/src/webserver/server.rs(1 hunks)
💤 Files with no reviewable changes (1)
- applications/tari_indexer/web_ui/src/routes/VN/Components/NftRow.tsx
🚧 Files skipped from review as they are similar to previous changes (32)
- crates/state_store_rocksdb/src/store.rs
- applications/tari_validator_node/src/bootstrap.rs
- crates/state_store_rocksdb/src/options.rs
- crates/template_lib_types/src/crypto/schnorr.rs
- crates/state_store_rocksdb/src/column_families/mod.rs
- crates/consensus_tests/src/substate_store.rs
- crates/consensus/src/messages/catch_up.rs
- applications/tari_validator_node/log4rs_sample.yml
- crates/consensus/src/hotstuff/error.rs
- crates/template_lib_types/src/crypto/ristretto.rs
- crates/consensus/src/hotstuff/common.rs
- crates/storage/src/consensus_models/block_header.rs
- crates/state_store_rocksdb/src/writer.rs
- crates/p2p/src/conversions/consensus.rs
- crates/storage/src/state_store/mod.rs
- crates/storage/src/consensus_models/no_vote.rs
- crates/consensus_types/src/bookkeeping/leaf_block.rs
- crates/consensus/src/hotstuff/on_propose.rs
- crates/consensus/src/hotstuff/vote_collector/proposal_collector.rs
- crates/consensus_tests/src/support/validator/builder.rs
- crates/consensus_tests/src/support/validator/instance.rs
- crates/consensus_types/src/certificates/quorum_certificate.rs
- crates/template_builtin/templates/faucet/src/lib.rs
- crates/consensus/src/messages/mod.rs
- applications/tari_validator_node/src/p2p/rpc/state_sync_task.rs
- crates/consensus/src/hotstuff/on_catch_up_sync.rs
- crates/state_store_rocksdb/src/reader.rs
- crates/consensus_types/src/bookkeeping/high_pc.rs
- crates/consensus/src/traits/block_store.rs
- crates/template_builtin/templates/nft_faucet/src/lib.rs
- applications/tari_walletd/web_ui/src/routes/AssetVault/NFTs/components/NftParts.tsx
- crates/consensus/src/hotstuff/block_change_set.rs
🧰 Additional context used
🧬 Code graph analysis (12)
crates/consensus/src/hotstuff/vote_collector/collector.rs (3)
crates/common_types/src/committee.rs (2)
quorum_threshold(65-67)quorum_threshold(253-255)crates/template_lib_types/src/crypto/ristretto.rs (1)
zero(27-29)crates/template_lib_types/src/crypto/schnorr.rs (2)
zero(22-27)signature(64-66)
crates/consensus/src/hotstuff/pacemaker.rs (2)
crates/consensus/src/hotstuff/worker.rs (1)
on_leader_timeout(648-679)crates/consensus/src/hotstuff/on_leader_timeout.rs (1)
leader_timed_out(46-48)
crates/consensus_tests/src/leader_failure.rs (2)
crates/consensus_tests/src/support/harness.rs (2)
builder(80-82)network(362-364)crates/consensus_tests/src/support/network.rs (1)
total_messages_sent(166-168)
crates/consensus/src/hotstuff/on_leader_timeout.rs (2)
crates/consensus/src/hotstuff/pacemaker.rs (1)
new(36-57)crates/consensus/src/hotstuff/pacemaker_handle.rs (1)
new(39-53)
crates/consensus/src/hotstuff/worker.rs (2)
crates/consensus/src/hotstuff/on_leader_timeout.rs (1)
wait(40-44)crates/consensus/src/hotstuff/on_inbound_message.rs (1)
next_message(35-57)
crates/consensus/src/hotstuff/on_catch_up_sync_request.rs (4)
crates/p2p/src/conversions/consensus.rs (15)
from(91-116)from(155-161)from(182-188)from(209-214)from(235-239)from(254-260)from(277-282)from(297-326)from(358-367)from(383-391)from(413-420)from(442-449)from(470-486)from(565-572)from(602-608)crates/consensus/src/messages/message.rs (1)
epoch(66-78)crates/consensus_types/src/bookkeeping/leaf_block.rs (2)
epoch(55-57)height(43-45)crates/storage/src/consensus_models/block.rs (1)
get_all_blocks_between(492-501)
crates/consensus/src/hotstuff/on_receive_local_proposal.rs (2)
crates/consensus/src/traits/certificate.rs (3)
get(21-21)get(41-43)get(102-104)crates/storage/src/consensus_models/block.rs (1)
get(463-465)
crates/consensus/src/hotstuff/on_inbound_message.rs (5)
crates/consensus/src/messages/message.rs (1)
epoch(66-78)crates/consensus_types/src/bookkeeping/high_pc.rs (2)
epoch(51-53)height(43-45)crates/consensus_types/src/bookkeeping/leaf_block.rs (2)
epoch(55-57)height(43-45)crates/consensus_types/src/certificates/quorum_certificate.rs (2)
epoch(33-38)height(40-45)crates/p2p/src/conversions/consensus.rs (15)
from(91-116)from(155-161)from(182-188)from(209-214)from(235-239)from(254-260)from(277-282)from(297-326)from(358-367)from(383-391)from(413-420)from(442-449)from(470-486)from(565-572)from(602-608)
crates/consensus/src/hotstuff/on_ready_to_vote_on_local_block.rs (2)
crates/storage/src/consensus_models/block_header.rs (2)
parent(379-381)total_accumulated_exhaust_burn(439-441)crates/storage/src/consensus_models/block.rs (2)
parent(323-325)header(225-227)
crates/consensus/src/traits/certificate.rs (2)
crates/consensus_types/src/bookkeeping/high_pc.rs (1)
height(43-45)crates/consensus_types/src/certificates/quorum_certificate.rs (1)
height(40-45)
crates/consensus/src/hotstuff/on_next_sync_view.rs (2)
bindings/src/types/Epoch.ts (1)
Epoch(3-3)bindings/src/types/NodeHeight.ts (1)
NodeHeight(3-3)
crates/template_lib_types/src/crypto/scalar.rs (4)
crates/template_lib_types/src/crypto/ristretto.rs (1)
zero(27-29)crates/template_lib_types/src/crypto/schnorr.rs (1)
zero(22-27)crates/engine_types/src/hash.rs (1)
zero(29-31)crates/template_lib_types/src/crypto/commitment.rs (1)
zero(34-36)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: check stable
- GitHub Check: clippy
- GitHub Check: check nightly
- GitHub Check: test
🔇 Additional comments (41)
networking/core/src/worker.rs (1)
69-69: LOG_TARGET rename looks good; just be aware of external log filtersUpdating
LOG_TARGETto"tari::networking::service::worker"is consistent and keeps all logging call sites intact. Only potential impact is on any external log routing or filtering that matches the old"tari::ootle::..."prefix, so ensure dashboards/filters are updated accordingly if needed.applications/tari_validator_node/src/p2p/rpc/rpc_impl.rs (1)
405-412: LGTM! Appropriate log level adjustment.Reducing the log level from
info!todebug!for peer-initiated sync events is appropriate. These are frequent operational events that don't require info-level visibility and the change reduces log verbosity while preserving diagnostic capability.applications/tari_validator_node/src/p2p/rpc/mod.rs (1)
24-27: LGTM! Consistent module rename.The module declaration and public re-export have been updated consistently from
service_impltorpc_impl. The public API surface remains unchanged asValidatorNodeRpcServiceImplis still exported.crates/consensus_types/src/certificates/mod.rs (1)
6-6: LGTM!The addition of the
quorum_certificatemodule follows the existing pattern in this file perfectly. The module declaration and wildcard re-export are consistent with how other certificate types are exposed, and the alphabetical ordering is maintained.Also applies to: 12-12
crates/template_lib_types/src/crypto/scalar.rs (1)
24-26: Const-ifyingScalar32Bytes::zerois correct and consistentMaking
zeroapub const fnwithSelf([0u8; Self::length()])only uses const-safe operations and matches the pattern used in other types (RistrettoPublicKeyBytes,SchnorrSignatureBytes, commitments, and hashes). This improves usability in const contexts without breaking existing runtime callers; just ensure your MSRV covers this const-eval pattern.utilities/db_inspector/src/webserver/server.rs (1)
113-114: LGTM! Correctly adds diagnostic no-vote column family route.The addition of
DiagnosticsNoVoteCfto the column family routes follows the established pattern and aligns with the PR objective to persist no-vote diagnostic data. Verification confirms thatDiagnosticsNoVoteCfis properly defined incrates/state_store_rocksdb/src/column_families/diagnostic_no_vote.rsand publicly exported, making the usage at lines 113-114 correct. The trailing comma on line 113 improves maintainability.applications/tari_indexer/web_ui/src/routes/VN/Components/Resources.tsx (1)
104-104: The review comment is based on incorrect technical premises and should be disregarded.After examining the codebase, the key finding is that
original_ownerdoes not exist anywhere in the NFT-related code. TheNftDatainterface in NftRow.tsx contains onlyimg,title,address,version,nft, andamount—there is nooriginal_ownerfield to remove. The destructuring at line 104 (const { name, amount } = nftData;) is consistent with the actual interface definition. The current commit "fix(consensus): more robust catch up sync" is indeed the stated PR objective, and Resources.tsx changes are part of that effort, not unrelated additions.The review comment appears to have been based on an inaccurate AI summary that incorrectly identified a field that was not present in the interface.
Likely an incorrect or invalid review comment.
crates/consensus/src/hotstuff/vote_collector/timeout_collector.rs (2)
16-16: LGTM: Logging namespace alignment.The LOG_TARGET update to the ootle consensus namespace is consistent with the broader refactoring across vote collector modules.
78-78: LGTM: Enhanced debugging for no-quorum case.The addition of a debug log when no quorum is reached improves observability without affecting the control flow.
crates/consensus/src/hotstuff/vote_collector/collector.rs (3)
17-17: LGTM: Logging namespace alignment.Consistent with the LOG_TARGET update in timeout_collector.rs, aligning with the ootle consensus namespace.
57-68: LGTM: Reduced log verbosity with quorum threshold displayed.The early calculation of
quorum_thresholdenables its inclusion in the debug log message, and the info→debug change appropriately reduces verbosity for frequent vote collection events.
73-82: LGTM: Consistent verbosity reduction.The info→debug change for below-threshold votes aligns with the broader logging verbosity reduction across the vote collection flow.
crates/consensus_tests/src/support/harness.rs (3)
653-654: LGTM! Extended cleanup intervals for test stability.Increasing these intervals from 60s to 1000s reduces the likelihood of cleanup operations interfering with catch-up synchronization tests. This aligns well with the PR's goal of testing robust catch-up behavior.
666-669: LGTM! Clean builder method for config customization.The new
modify_configmethod follows the builder pattern consistently and enables fine-grained configuration adjustments in tests.
264-264: LGTM! API update aligns with network.rs refactoring.The simplified call reflects the network module's move from destination-based to address-based offline tracking.
crates/consensus_tests/src/leader_failure.rs (6)
4-4: LGTM! Import supports new test timing.
41-41: LGTM! Consistent API usage.Updated to use the new address-based
go_offlinesignature.
115-116: LGTM! Consistent API usage.Updated to use the new address-based
go_offlinesignature for both nodes.
190-190: LGTM! Consistent API usage.Updated to use the new address-based
go_offlinesignature.
309-309: LGTM! Consistent API usage.Updated to use the new address-based
go_offlinesignature.
354-426: Verify clean shutdown expectation for recovered node.The test brings
failure_nodeback online at Line 398, then waits for ALL validators (includingfailure_node) to commit all transactions (Lines 410-415). However, Line 425 excludesfailure_nodefrom the clean shutdown assertion.If the node successfully catches up and commits all transactions (which the loop requires to exit), it should be able to shut down cleanly. Consider using
assert_clean_shutdown()instead, or clarify why the recovered node is expected to have shutdown issues.crates/consensus_tests/src/support/network.rs (7)
11-11: LGTM! Import supports new logging.
116-116: LGTM! Simplified offline tracking.Changing from destination-based to address-based storage makes the offline logic more precise and easier to reason about.
131-145: LGTM! Clean online/offline management.The address-based implementation is straightforward and includes appropriate error handling. The new
go_onlinemethod properly validates that the node was actually offline before attempting to bring it back online.
147-150: LGTM! Simplified offline check.The address-based lookup is much cleaner than the previous implementation.
175-175: LGTM! Necessary trait implementations.Adding
PartialEqandEqsupports the new address-based comparison logic.
337-337: LGTM! Updated broadcast filtering.Correctly uses the new address-based offline check.
365-379: LGTM! Updated message filtering with clear logging.The implementation correctly drops messages when either sender or receiver is offline, with helpful diagnostic logging at Line 366.
crates/consensus/src/traits/certificate.rs (2)
78-93: LGTM on the logging update.The change from
block_height()toheight()in the logging at line 85 is consistent with the API refactor and correctly reflects the new method name.
64-76: Comment wording on lines 65-67 is misleading; TimeoutCertificate design is intentionally different.The API rename from
block_height()toheight()is correct and consistent throughout. However, the comment at lines 65-67 is confusing—it says "If we receive a new high PC" but this code executes in the branch where the PC is not higher (high_pc.height() >= self.height()).Suggest rewording to clarify the intent: "If we receive a high PC at or above our current height, clear the last sent new view to ensure subsequent NEWVIEWs align with network progress after potential offline period."
Regarding
TimeoutCertificate::update_highest(lines 119-148): it intentionally lackslast_sent_new_view_clear(). The clearing for timeout certificates happens at a higher level inon_receive_new_view.rswhen a new TC is confirmed, whereas ProposalCertificate clears preemptively when seeing any high PC (even if non-new). This asymmetry is by design and serves different recovery semantics for each certificate type.crates/consensus/src/hotstuff/on_receive_vote.rs (1)
58-61: LGTM! API change aligns with broader refactoring.The variable rename from
high_qctohigh_pcand the updated call toreset_leader_timeout(&high_pc)are consistent with the broader API changes wherereset_leader_timeoutnow accepts a&HighPcreference instead of extracting the height directly.crates/consensus/src/hotstuff/on_receive_local_proposal.rs (1)
812-833: New parked block detection aligns with PR objectives.The new logic correctly detects when a justify block is parked and returns a specific error variant, which prevents the catch-up loop described in the PR objectives. The warning message on line 818 appropriately notes this case shouldn't happen in normal operation due to message buffering.
However, ensure this error is properly handled in the match statement at lines 728-730 (see separate comment).
crates/consensus/src/hotstuff/pacemaker.rs (3)
88-88: LGTM! Timeout counter added for consecutive timeout tracking.The new
num_timeoutscounter properly tracks consecutive leader timeouts, is reset on successful pacemaker reset (line 103), and is incremented when timeouts occur (lines 157, 186). This aligns with the broader timeout payload enrichment.
157-162: LGTM! Enriched LeaderTimeout payload on resume.The code correctly increments the timeout counter and sends a
LeaderTimeoutpayload with comprehensive context (current_height,current_high_pc,num_timeouts) instead of just a height value. This aligns with the new timeout signaling API.
186-191: LGTM! Enriched LeaderTimeout payload on timeout.Consistent with the resume path, this correctly sends the enriched
LeaderTimeoutpayload when a timeout occurs naturally, providing downstream handlers with full timeout context.crates/consensus/src/hotstuff/on_leader_timeout.rs (2)
9-22: LGTM! Well-designed timeout context struct.The new
LeaderTimeoutstruct provides comprehensive timeout context withcurrent_height,current_high_pc, andnum_timeoutsfields. Thedelta()helper method is a useful addition for computing the height difference. The struct derives appropriate traits and aligns with the updated timeout signaling across pacemaker and worker modules.
27-48: LGTM! API updated to use enriched timeout payload.The channel types and method signatures correctly transition from passing raw
NodeHeightto the enrichedLeaderTimeoutpayload, maintaining backward compatibility through proper initialization and error handling.crates/consensus/src/messages/message.rs (3)
18-41: LGTM! Clean renaming and documentation improvements.The rename from
SyncRequestMessagetoCatchUpRequestMessageis more descriptive and aligns with the broader catch-up refactoring. The added documentation comments improve code clarity for each message variant.
87-92: LGTM! Useful helper method.The
into_proposalmethod provides a convenient way to extract the innerProposalMessageby value, complementing the existingproposal()method that returns a reference.
96-147: LGTM! Consistent enum variant referencing.Refactoring the
Displayimplementation to useSelf::consistently for all match arms improves code style and maintainability.crates/consensus/src/hotstuff/on_ready_to_vote_on_local_block.rs (1)
421-431: TheNoVoteReason::TotalExhaustBurnDisagreementvariant is properly defined and integrated.Verification confirms that the
TotalExhaustBurnDisagreementvariant exists in theNoVoteReasonenum (defined incrates/storage/src/consensus_models/no_vote.rs:57) and is correctly referenced in the display formatting logic. The usage in the reviewed code is valid.
67b43a1 to
f3472d1
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/engine_types/src/substate.rs (1)
237-303: Const predicates look fine; updateis_rootcomment to match behavior
- Making these simple
matches!/forwarding predicatesconstis safe and consistent with their implementations.is_root’s body now treatsClaimedOutputTombstoneas root:matches!(self, Self::Component(_) | Self::Utxo(_) | Self::ClaimedOutputTombstone(_))but the comment still says “A component and utxo are ‘root’ substates…”. That’s now stale and can surprise readers.
Suggest aligning the comment, e.g.:
- // A component and utxo are "root" substates i.e. they may not have a parent node. NOTE: this concept isn't - // well-defined right now, this is simply used to prevent components being detected as dangling. + // Components, UTXOs and claimed output tombstones are "root" substates i.e. they may not have a parent node. + // NOTE: this concept isn't well-defined right now; this is simply used to prevent certain substates being + // detected as dangling.Everything else in this block reads correctly.
crates/consensus/src/hotstuff/on_ready_to_vote_on_local_block.rs (1)
1169-1180: AllAccept DecisionDisagreement reports the wrong “local” decisionIn the AllAccept path:
if tx_rec.current_decision().is_abort() { // ... return Ok(Some(NoVoteReason::DecisionDisagreement { local: atom.decision, remote: Decision::Commit, })); }At this point
tx_rec.current_decision().is_abort()is true andatom.decisionmust beCommit(earlier guard), so theDecisionDisagreementshould record:
local: the actual local decision (tx_rec.current_decision()), i.e. Abortremote: the leader’s decision (atom.decision), i.e. CommitOtherwise diagnostics and any consumers of
NoVoteReason::DecisionDisagreementwill see both sides asCommit.Suggested fix:
- return Ok(Some(NoVoteReason::DecisionDisagreement { - local: atom.decision, - remote: Decision::Commit, - })); + return Ok(Some(NoVoteReason::DecisionDisagreement { + local: tx_rec.current_decision(), + remote: Decision::Commit, + }));
♻️ Duplicate comments (2)
crates/consensus/src/hotstuff/on_receive_local_proposal.rs (1)
728-730: Critical:JustifyBlockParkederror not handled in the error matching logic.The code returns
ProposalValidationError::JustifyBlockParkedat line 819, but the error handling at lines 728-730 only explicitly matchesJustifyBlockNotFound:Err(err @ HotStuffError::ProposalValidationError(ProposalValidationError::JustifyBlockNotFound { .. })) => { Err(err) },When
JustifyBlockParkedis returned, it does not match this pattern and falls through to different error handling logic at line 732, bypassing the intended catch-up sync path.Add
JustifyBlockParkedto the match using|or a separate arm:-Err(err @ HotStuffError::ProposalValidationError(ProposalValidationError::JustifyBlockNotFound { .. })) => { +Err(err @ HotStuffError::ProposalValidationError( + ProposalValidationError::JustifyBlockNotFound { .. } | ProposalValidationError::JustifyBlockParked { .. } +)) => { Err(err) },crates/consensus/src/hotstuff/on_next_sync_view.rs (1)
32-32: Critical: In-memory cache invalidation missing when storage is cleared.The in-memory cache
last_sent_new_viewis never cleared when storage is invalidated. Whenhigh_pc_set()orhigh_tc_set()callslast_sent_new_view_clear()(atcertificate.rs:68andon_receive_new_view.rs:164), only storage is cleared while the in-memory cache persists. This allows stale height data to be reused within the same epoch.Scenario demonstrating the bug:
- Send NEWVIEW at height 10 in epoch 1 → cache =
Some((1, 10)), storage persists- High PC received clears storage → storage = empty, cache =
Some((1, 10))still set- Next timeout at height 9 → cache check passes (same epoch, 10 >= 9), incorrectly skips to height 11
Implement bidirectional cache invalidation: clear
self.last_sent_new_view = Nonewheneverstorage.last_sent_new_view_clear()is called.Also applies to: 47-47, 64-68, 120-120
🧹 Nitpick comments (22)
crates/consensus_types/src/bookkeeping/leaf_block.rs (1)
39-41: LGTM! Consider adding a doc comment for clarity.The implementation correctly identifies blocks at height 0. Since
LeafBlockincludesepochandshard_groupfields, a brief doc comment clarifying whether "genesis" means "first block of the chain" or "first block of this shard/epoch" would improve readability.Example:
+ /// Returns `true` if this block is at height 0 (genesis height). pub fn is_genesis(&self) -> bool { self.height.is_zero() }crates/consensus/src/hotstuff/vote_collector/collector.rs (2)
84-91: Consider keeping QUORUM achievement atinfo!level for operational visibility.While reducing log verbosity for vote collection is reasonable, the QUORUM achievement message represents a significant consensus milestone. Demoting it to
debug!may reduce operational visibility into consensus progress without requiring verbose debug-level logging across the system.Consider keeping this specific log at
info!level:- debug!( + info!( target: LOG_TARGET, "🔥 Received {} from {} ({} of {}). QUORUM!", vote_display, sender_hash, threshold_decision.total_power, quorum_threshold );
238-325: Good addition of unit tests for vote storage.The test module provides solid coverage of the core
save_votefunctionality, including successful save and duplicate detection. The use of zero constants for cryptographic fields inTestVoteis appropriate for unit testing.For more comprehensive coverage, consider adding tests for
calculate_threshold_decisionto verify:
- Quorum threshold calculation with Accept votes
- Quorum threshold calculation with Reject votes
- Mixed voting scenarios where quorum is not reached
utilities/tariswap_test_bench/src/tariswap.rs (1)
107-108: Consider renaming the inner loop variable to avoid shadowing.The inner loop variable
ifromenumerate()shadows the outer loop variablei. While functionally correct (the innericorrectly represents the global index), this shadowing is confusing and could lead to maintenance errors.Apply this diff for clarity:
- for (i, tariswap) in tariswaps.iter().enumerate().skip(i * BATCH_SIZE).take(BATCH_SIZE) { - let account = &accounts[i % accounts.len()]; + for (idx, tariswap) in tariswaps.iter().enumerate().skip(i * BATCH_SIZE).take(BATCH_SIZE) { + let account = &accounts[idx % accounts.len()];crates/engine_types/src/substate.rs (1)
186-191: Consider returning a reference fromas_utxo_addressto avoid cloning
as_utxo_addresscurrently allocates by cloning the innerUtxoAddressand therefore cannot beconst, unlike the other accessors.If most call sites only need a borrow (like
as_non_fungible_address), consider:- pub fn as_utxo_address(&self) -> Option<UtxoAddress> { - match self { - Self::Utxo(address) => Some(address.clone()), - _ => None, - } - } + pub const fn as_utxo_address(&self) -> Option<&UtxoAddress> { + match self { + Self::Utxo(address) => Some(address), + _ => None, + } + }This avoids cloning and keeps the API const-friendly, at the cost of updating callers.
crates/consensus_tests/src/support/validator/builder.rs (3)
155-162: Debugging data enabled on RocksDB test store looks appropriateWiring
DatabaseOptions::default().with_debugging_data(true)intoTestStore::openin the test harness is a sensible place to turn on the extra diagnostics, and keeps production code unaffected. If this builder ever gets reused for performance‑sensitive benchmarks, consider making debugging data a configurable flag onValidatorBuilderinstead of hard‑coding it here.
164-168: XTR genesis injection is correct in spirit; consider centralizing config useInjecting the stealth Tari resource via
get_stealth_tari_resource(self.config.as_ref().unwrap().network)andcreate_substatematches the “implicit XTR” assumption and will prevent tests from failing due to a missing fee resource. You’re unwrappingconfighere and again later for the HotStuff worker; you might simplify by bindinglet config = self.config.clone().expect(...)once and reusing it for both network and worker construction to avoid multiple panics and repeatedunwrap/expectcalls.
237-257: Consider idempotence and API shape ofcreate_substate
create_substatenicely encapsulates the batch construction, but a few details are worth double‑checking:
- It always pushes an
Up { version: 0 }inEpoch::zero(). If the same RocksDB path is reused across multiple spawns or restarts, and the underlying storage treatsUpat an existing(substate_id, 0)as an error, this could fail on subsequent runs; you may want a “create‑if‑missing” guard here or document that the store must be fresh.- The extra bounds
TTx: Deref<Target = _>andTTx::Addr: NodeAddressable + Serializeare unused in the current body; either use them (e.g., to check for existing state via a read before inserting) or simplify the signature to the minimal constraints to keep the helper easy to reuse.- Hard‑coding
Epoch::zero()and version 0 makes this effectively a “genesis substate” helper; if you expect broader use, consider parameterizing epoch/version or renaming it to reflect that it’s for genesis/default state only.crates/wallet/sdk/src/apis/transaction.rs (3)
324-335: Claimed tombstone down-guard and lowered log level look reasonableThe early
id.is_claimed_output_tombstone()check plus the warn-and-continue behavior makes sense given the expectation that these should never be downed, and downgrading the “downed substate not found” case todebug!should reduce noise when state is partially present. The only trade‑off is that, if a tombstone ever does appear in adowndiff, we’ll silently leave any existing record in the DB; if that would indicate actual corruption, consider adding a stronger assertion/metric in addition to the log in future.
344-354: Component path refactor tosubstate_value().component()is consistent, but stillunwrapsUsing
substate.substate_value().component()andcomponent.state()here lines up with the updated API, and propagatingmodule_name/template_addressfrom the component itself keeps the metadata intact. You still rely onunwrap()on Line 345; if malformed diffs from the network are a realistic failure mode for wallets, you might want to turn this into a fallible path (e.g., map to anInvalidTransactionQueryResponse) instead of panicking, but that’s optional given the invariants elsewhere (e.g.,SubstateId::Component(_) => unreachable!()later).
446-460: Intentionally dropping root upsert for vaults without a known parentSwitching the
Nonebranch fortx.vaults_get(vault_id)to just adebug!and not callingsubstates_upsert_rootanymore means the wallet will no longer index vaults it doesn’t recognize as belonging to one of its accounts, which should reduce clutter from other parties’ vaults. This does change behavior versus the previous fallback “root vault” upsert. If there are no flows that rely on discovering new accounts via such stray vaults, this seems like the right direction; otherwise, we’d want either explicit tests around that behavior or a short comment explaining why it’s safe to ignore them and maybe cleaning up the commented‑out block once you’re confident in the new semantics.crates/consensus_tests/src/support/network.rs (2)
116-150: Offline node tracking works butgo_offlineis not idempotentStoring offline nodes in
Vec<TestAddress>and querying viais_offlineis fine for tests, but note that:
go_offlineblindlypushes the node, so repeated calls add duplicates.go_onlineonly removes the first occurrence, so a node can remain effectively offline if it was taken offline more than once.If you ever re‑use
go_offlinein loops or helper utilities, this can bite. Suggest making the set idempotent, e.g. by checkingcontainsbeforepushor by switching toHashSet<TestAddress>and usinginsert/remove.
337-379: Offline filtering in worker is clear; semantics worth notingThe new
is_offline_destination(&from, &to)returns true if either endpoint is offline, and bothhandle_broadcastandhandle_leaderdrop messages wheneverfrom != toand this is true. That means:
- Messages to/from offline nodes are dropped, as expected.
- Self‑messages (
from == to) are always allowed, even if that node is marked offline.Given this is a test network, these semantics seem intentional and match the higher‑level tests, but if you ever want a stricter “fully isolated” offline mode, you may also want to block self‑messages.
crates/consensus_tests/src/leader_failure.rs (1)
354-426: Catch‑up test looks good; watch out for timing sensitivity and shutdown exceptionThe new
single_shard_node_goes_down_and_catches_uptest:
- Exercises the intended scenario: node 4 goes offline mid‑epoch, more transactions are submitted, then it returns and must commit all
tx_ids.- Uses
Instantwith 2s/13s thresholds and a 5s block time, plus a 60s overall event timeout andheight <= 50guard, which should be safe but is still somewhat time‑sensitive under heavy CI load.Two small suggestions:
- Consider documenting the 2s/13s choices (e.g., relation to pacemaker block time and missed‑proposal thresholds) so future maintainers understand the intended phases.
- In this particular test the node is brought back online and expected to behave normally; you might not need to exclude
failure_nodeinassert_clean_shutdown_except, and asserting a clean shutdown for it as well would slightly strengthen the test.Otherwise the logic and assertions look solid and clearly exercise the catch‑up path.
crates/consensus/src/hotstuff/pacemaker.rs (1)
75-88: Timeout counter semantics: confirm reset conditionsThe
num_timeoutscounter andLeaderTimeoutpayload wiring look sound: it’s reset onReset, incremented only when a timeout is actually delivered (including the deferred case when resuming from suspension), and carriescurrent_heightandcurrent_high_pccorrectly.One nuance:
StopandStartdo not resetnum_timeouts, so ifStartis ever called again without an interveningReset, the worker will see a continuing timeout count. If the intended contract is “consecutive timeouts since last pacemaker start/epoch/catch-up”, you may want to also clearnum_timeoutsonStart(and possiblyStop), or at least document thatResetis the only semantic reset.Also applies to: 99-106, 141-166, 176-193
crates/state_store_tests/src/blocks.rs (1)
5-5: Tests’ migration to PcId matches storage API expectationsThe updates to use
PcId(includingPcId::zero()inblocks_set_qcscalls) are consistent with the new consensus/storage interfaces, and the tests still validate the intended behaviors (has_justify_qc,is_committed, committed-by-parent queries, etc.).If you ever want these tests to catch mismatches between stored IDs and certificate IDs, you could switch from
PcId::zero()to actualProposalCertificate::calculate_id()values, but that’s an optional strengthening, not required for this PR.Also applies to: 64-65, 72-73, 182-185, 232-233, 258-259, 285-286
crates/state_store_rocksdb/src/writer.rs (1)
120-156: Debug‑only persistence for pool history and no‑vote diagnosticsGating both the transaction‑pool debug history and the new
DiagnosticsNoVoteCfwrites onoptions.debugging_datais a good separation between normal operation and diagnostics, anddiagnostics_add_no_votecorrectly degrades to a no‑op when debugging is off. One thing to consider (not urgent) is whether failures in these debug CFs should be logged and ignored rather than bubbling up, since they are non‑critical.Also applies to: 859-868, 1726-1738
crates/consensus/src/hotstuff/on_leader_timeout.rs (1)
9-22:LeaderTimeoutstruct cleanly enriches timeout signallingMoving from a raw
NodeHeighttoLeaderTimeoutover the watch channel gives the pacemaker richer context (height, high_pc, num_timeouts) while preserving the simplewait()/leader_timed_out()API. Thedelta()helper is safe viasaturating_suband is a useful primitive for backoff logic.Also applies to: 27-48
crates/state_store_rocksdb/src/reader.rs (1)
1102-1123: PcId‑based proposal certificate getters look consistentThe switch of
proposal_certificates_get/proposal_certificates_get_manytoPcIdmatches the trait signatures incrates/storage/src/state_store/mod.rs, andmulti_get+ exact‑length check preserves previous guarantees (all or NotFound).One small nit: the error
item: "QuorumCertificate"and key"one or more qc_ids"are now slightly misleading since this API is specifically about proposal certificates; consider updating the strings when convenient.crates/consensus/src/hotstuff/on_inbound_message.rs (1)
238-257: Acknowledged workaround for genesis startup edge case.The special-case logic for
has_processed_first_block == falseat lines 245–250 is tightly coupled to specific height values and marked "hacky" by the developer. While it addresses the parked-first-block scenario described in the PR objectives, this code is fragile and may break if the startup sequence changes.Consider capturing the intent more explicitly in follow-up work, e.g., by tracking "genesis startup phase" separately or generalizing the logic to handle any buffered block that depends on an unprocessed predecessor.
crates/consensus/src/hotstuff/worker.rs (2)
667-675: Consider documenting the timeout threshold.The choice of 3 consecutive timeouts as the trigger for catch-up sync (line 667) appears reasonable, but documenting the rationale or making it configurable would improve maintainability.
1147-1151: Replace magic number with a named constant.The
NodeHeight(99)at line 1150 is based on a batch size of 100, but hardcoding it here creates a maintenance risk if the batch size changes.Consider defining a constant:
const CATCH_UP_BATCH_SIZE: u64 = 100;Then use:
self.worker_state.catch_up = Some(CatchUp { high_qc: remote_height, - expected_batch_height: current_height + NodeHeight(99), + expected_batch_height: current_height + NodeHeight(CATCH_UP_BATCH_SIZE - 1), });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (101)
applications/tari_indexer/web_ui/src/routes/VN/Components/NftRow.tsx(0 hunks)applications/tari_indexer/web_ui/src/routes/VN/Components/Resources.tsx(1 hunks)applications/tari_validator_node/log4rs_sample.yml(2 hunks)applications/tari_validator_node/src/bootstrap.rs(1 hunks)applications/tari_validator_node/src/p2p/rpc/mod.rs(1 hunks)applications/tari_validator_node/src/p2p/rpc/rpc_impl.rs(1 hunks)applications/tari_validator_node/src/p2p/rpc/state_sync_task.rs(1 hunks)applications/tari_walletd/Cargo.toml(1 hunks)applications/tari_walletd/src/handlers/accounts.rs(0 hunks)applications/tari_walletd/src/handlers/validator.rs(0 hunks)applications/tari_walletd/src/services/template_monitor.rs(2 hunks)applications/tari_walletd/web_ui/src/routes/AssetVault/NFTs/components/NftParts.tsx(2 hunks)clients/tari_indexer_client/src/rest_api_client.rs(2 hunks)clients/tari_indexer_client/src/types.rs(0 hunks)crates/common_types/src/versioned_substate_id.rs(1 hunks)crates/consensus/src/hotstuff/block_change_set.rs(3 hunks)crates/consensus/src/hotstuff/common.rs(4 hunks)crates/consensus/src/hotstuff/error.rs(3 hunks)crates/consensus/src/hotstuff/foreign_proposal_processor.rs(2 hunks)crates/consensus/src/hotstuff/on_catch_up_sync.rs(3 hunks)crates/consensus/src/hotstuff/on_catch_up_sync_request.rs(5 hunks)crates/consensus/src/hotstuff/on_inbound_message.rs(5 hunks)crates/consensus/src/hotstuff/on_leader_timeout.rs(1 hunks)crates/consensus/src/hotstuff/on_next_sync_view.rs(5 hunks)crates/consensus/src/hotstuff/on_propose.rs(1 hunks)crates/consensus/src/hotstuff/on_ready_to_vote_on_local_block.rs(14 hunks)crates/consensus/src/hotstuff/on_receive_local_proposal.rs(5 hunks)crates/consensus/src/hotstuff/on_receive_new_view.rs(1 hunks)crates/consensus/src/hotstuff/on_receive_vote.rs(1 hunks)crates/consensus/src/hotstuff/pacemaker.rs(6 hunks)crates/consensus/src/hotstuff/pacemaker_handle.rs(3 hunks)crates/consensus/src/hotstuff/vote_collector/collector.rs(5 hunks)crates/consensus/src/hotstuff/vote_collector/proposal_collector.rs(1 hunks)crates/consensus/src/hotstuff/vote_collector/timeout_collector.rs(2 hunks)crates/consensus/src/hotstuff/worker.rs(20 hunks)crates/consensus/src/messages/catch_up.rs(1 hunks)crates/consensus/src/messages/message.rs(5 hunks)crates/consensus/src/messages/mod.rs(1 hunks)crates/consensus/src/traits/block_store.rs(1 hunks)crates/consensus/src/traits/certificate.rs(5 hunks)crates/consensus/src/validations/block.rs(2 hunks)crates/consensus/src/validations/common.rs(5 hunks)crates/consensus_tests/Cargo.toml(1 hunks)crates/consensus_tests/src/leader_failure.rs(6 hunks)crates/consensus_tests/src/substate_store.rs(1 hunks)crates/consensus_tests/src/support/harness.rs(3 hunks)crates/consensus_tests/src/support/network.rs(8 hunks)crates/consensus_tests/src/support/transaction_executor.rs(2 hunks)crates/consensus_tests/src/support/validator/builder.rs(3 hunks)crates/consensus_tests/src/support/validator/instance.rs(1 hunks)crates/consensus_types/src/bookkeeping/high_pc.rs(1 hunks)crates/consensus_types/src/bookkeeping/leaf_block.rs(1 hunks)crates/consensus_types/src/certificates/mod.rs(1 hunks)crates/consensus_types/src/certificates/proposal_certificate.rs(4 hunks)crates/consensus_types/src/certificates/quorum_certificate.rs(1 hunks)crates/consensus_types/src/ids/mod.rs(1 hunks)crates/engine_types/src/published_template.rs(1 hunks)crates/engine_types/src/substate.rs(3 hunks)crates/engine_types/src/transaction_receipt.rs(1 hunks)crates/engine_types/src/validator_fee.rs(1 hunks)crates/p2p/src/conversions/consensus.rs(4 hunks)crates/state_store_rocksdb/src/column_families/certificates.rs(2 hunks)crates/state_store_rocksdb/src/column_families/diagnostic_no_vote.rs(1 hunks)crates/state_store_rocksdb/src/column_families/mod.rs(1 hunks)crates/state_store_rocksdb/src/options.rs(1 hunks)crates/state_store_rocksdb/src/reader.rs(6 hunks)crates/state_store_rocksdb/src/store.rs(2 hunks)crates/state_store_rocksdb/src/writer.rs(5 hunks)crates/state_store_tests/src/blocks.rs(6 hunks)crates/state_store_tests/src/helpers.rs(2 hunks)crates/state_store_tests/src/misc.rs(2 hunks)crates/state_store_tests/src/state_tree_diff.rs(2 hunks)crates/state_store_tests/src/transactions.rs(2 hunks)crates/storage/src/consensus_models/block.rs(8 hunks)crates/storage/src/consensus_models/block_header.rs(8 hunks)crates/storage/src/consensus_models/evidence.rs(6 hunks)crates/storage/src/consensus_models/no_vote.rs(2 hunks)crates/storage/src/state_store/mod.rs(4 hunks)crates/template_builtin/templates/faucet/src/lib.rs(3 hunks)crates/template_builtin/templates/nft_faucet/src/lib.rs(1 hunks)crates/template_lib/src/models/claimed_output_tombstone.rs(1 hunks)crates/template_lib/src/models/component.rs(1 hunks)crates/template_lib/src/models/resource.rs(1 hunks)crates/template_lib/src/models/vault.rs(1 hunks)crates/template_lib_types/src/crypto/commitment.rs(2 hunks)crates/template_lib_types/src/crypto/ristretto.rs(2 hunks)crates/template_lib_types/src/crypto/scalar.rs(1 hunks)crates/template_lib_types/src/crypto/schnorr.rs(1 hunks)crates/template_lib_types/src/entity_id.rs(1 hunks)crates/transaction/src/v1/transaction.rs(4 hunks)crates/transaction/src/v1/unsealed.rs(1 hunks)crates/wallet/sdk/src/apis/stealth_transfer/api.rs(3 hunks)crates/wallet/sdk/src/apis/transaction.rs(3 hunks)crates/wallet/sdk/src/local_key_store.rs(1 hunks)crates/wallet/sdk_services/src/indexer_rest_api.rs(1 hunks)integration_tests/tests/steps/wallet_daemon.rs(0 hunks)networking/core/src/worker.rs(1 hunks)utilities/db_inspector/src/webserver/server.rs(1 hunks)utilities/tariswap_test_bench/src/accounts.rs(2 hunks)utilities/tariswap_test_bench/src/main.rs(1 hunks)utilities/tariswap_test_bench/src/tariswap.rs(2 hunks)
💤 Files with no reviewable changes (5)
- applications/tari_walletd/src/handlers/validator.rs
- clients/tari_indexer_client/src/types.rs
- applications/tari_indexer/web_ui/src/routes/VN/Components/NftRow.tsx
- integration_tests/tests/steps/wallet_daemon.rs
- applications/tari_walletd/src/handlers/accounts.rs
🚧 Files skipped from review as they are similar to previous changes (22)
- crates/storage/src/consensus_models/no_vote.rs
- applications/tari_validator_node/src/bootstrap.rs
- crates/state_store_rocksdb/src/column_families/mod.rs
- crates/state_store_rocksdb/src/options.rs
- crates/consensus/src/messages/mod.rs
- crates/consensus/src/hotstuff/vote_collector/timeout_collector.rs
- crates/consensus/src/hotstuff/on_receive_vote.rs
- applications/tari_validator_node/src/p2p/rpc/state_sync_task.rs
- crates/state_store_rocksdb/src/store.rs
- applications/tari_validator_node/src/p2p/rpc/mod.rs
- applications/tari_validator_node/src/p2p/rpc/rpc_impl.rs
- crates/state_store_rocksdb/src/column_families/diagnostic_no_vote.rs
- crates/consensus/src/hotstuff/pacemaker_handle.rs
- crates/consensus/src/hotstuff/vote_collector/proposal_collector.rs
- applications/tari_indexer/web_ui/src/routes/VN/Components/Resources.tsx
- crates/consensus/src/messages/message.rs
- networking/core/src/worker.rs
- crates/consensus_types/src/bookkeeping/high_pc.rs
- crates/storage/src/state_store/mod.rs
- crates/consensus/src/hotstuff/block_change_set.rs
- applications/tari_walletd/web_ui/src/routes/AssetVault/NFTs/components/NftParts.tsx
- crates/template_builtin/templates/faucet/src/lib.rs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-19T13:41:29.551Z
Learnt from: leet4tari
Repo: tari-project/tari-ootle PR: 1650
File: .github/workflows/build_dockers_workflow.yml:56-77
Timestamp: 2025-11-19T13:41:29.551Z
Learning: The ootle repository does not use a `tari_network` parameter in its Docker build workflows, unlike other Tari repositories. The `build-matrix.sh` script in `buildtools/docker_rig/` only accepts three parameters: build_items, version, and platforms.
Applied to files:
crates/consensus_tests/Cargo.toml
📚 Learning: 2025-11-11T07:43:03.991Z
Learnt from: sdbondi
Repo: tari-project/tari-ootle PR: 1639
File: applications/tari_app_utilities/src/shared_consts.rs:6-7
Timestamp: 2025-11-11T07:43:03.991Z
Learning: In the Tari codebase, XTR uses micro-units where 1 XTR = 1,000,000 micro XTR. When reviewing Amount values, u64::MAX in micro XTR equals approximately 18.45 trillion whole XTR, not quintillion.
Applied to files:
crates/transaction/src/v1/transaction.rs
🧬 Code graph analysis (47)
crates/engine_types/src/transaction_receipt.rs (6)
crates/engine_types/src/published_template.rs (1)
as_object_key(59-61)crates/engine_types/src/validator_fee.rs (1)
as_object_key(47-49)crates/template_lib/src/models/claimed_output_tombstone.rs (1)
as_object_key(38-40)crates/template_lib/src/models/component.rs (1)
as_object_key(51-53)crates/template_lib/src/models/resource.rs (1)
as_object_key(47-49)crates/template_lib/src/models/vault.rs (1)
as_object_key(82-84)
utilities/tariswap_test_bench/src/tariswap.rs (1)
utilities/tariswap_test_bench/src/timer.rs (1)
info(32-34)
crates/template_lib/src/models/vault.rs (6)
crates/engine_types/src/published_template.rs (1)
as_object_key(59-61)crates/engine_types/src/transaction_receipt.rs (1)
as_object_key(56-58)crates/engine_types/src/validator_fee.rs (1)
as_object_key(47-49)crates/template_lib/src/models/claimed_output_tombstone.rs (1)
as_object_key(38-40)crates/template_lib/src/models/component.rs (1)
as_object_key(51-53)crates/template_lib/src/models/resource.rs (1)
as_object_key(47-49)
crates/consensus_tests/src/substate_store.rs (2)
crates/state_store_rocksdb/src/store.rs (1)
open(150-168)crates/state_store_rocksdb/src/options.rs (1)
default(33-39)
crates/consensus/src/hotstuff/foreign_proposal_processor.rs (3)
bindings/src/types/Decision.ts (1)
Decision(4-4)bindings/src/types/ProposalCertificate.ts (1)
ProposalCertificate(7-15)bindings/src/types/ValidatorSignatureBytes.ts (1)
ValidatorSignatureBytes(5-5)
crates/engine_types/src/published_template.rs (6)
crates/engine_types/src/transaction_receipt.rs (1)
as_object_key(56-58)crates/engine_types/src/validator_fee.rs (1)
as_object_key(47-49)crates/template_lib/src/models/claimed_output_tombstone.rs (1)
as_object_key(38-40)crates/template_lib/src/models/component.rs (1)
as_object_key(51-53)crates/template_lib/src/models/resource.rs (1)
as_object_key(47-49)crates/template_lib/src/models/vault.rs (1)
as_object_key(82-84)
crates/consensus/src/hotstuff/on_receive_new_view.rs (1)
crates/consensus/src/hotstuff/foreign_proposal_processor.rs (1)
qc(792-804)
crates/engine_types/src/validator_fee.rs (6)
crates/engine_types/src/published_template.rs (1)
as_object_key(59-61)crates/engine_types/src/transaction_receipt.rs (1)
as_object_key(56-58)crates/template_lib/src/models/claimed_output_tombstone.rs (1)
as_object_key(38-40)crates/template_lib/src/models/component.rs (1)
as_object_key(51-53)crates/template_lib/src/models/resource.rs (1)
as_object_key(47-49)crates/template_lib/src/models/vault.rs (1)
as_object_key(82-84)
crates/consensus_tests/src/leader_failure.rs (2)
crates/consensus_tests/src/support/harness.rs (2)
builder(80-82)network(362-364)crates/consensus_tests/src/support/network.rs (1)
total_messages_sent(166-168)
crates/consensus/src/hotstuff/on_propose.rs (6)
crates/consensus_types/src/bookkeeping/high_pc.rs (1)
height(43-45)crates/consensus_types/src/certificates/quorum_certificate.rs (1)
height(61-66)crates/storage/src/consensus_models/block.rs (1)
height(348-350)crates/storage/src/consensus_models/block_header.rs (1)
height(387-389)crates/consensus/src/hotstuff/vote_collector/collector.rs (1)
height(285-287)crates/consensus_types/src/bookkeeping/leaf_block.rs (1)
height(43-45)
crates/consensus/src/hotstuff/on_next_sync_view.rs (2)
bindings/src/types/Epoch.ts (1)
Epoch(3-3)bindings/src/types/NodeHeight.ts (1)
NodeHeight(3-3)
crates/consensus/src/hotstuff/on_receive_local_proposal.rs (3)
crates/storage/src/consensus_models/block.rs (2)
genesis(173-204)get(463-465)crates/storage/src/consensus_models/block_header.rs (1)
genesis(167-195)crates/consensus/src/traits/certificate.rs (3)
get(21-21)get(41-43)get(102-104)
crates/state_store_tests/src/transactions.rs (2)
crates/storage/src/consensus_models/block.rs (1)
zero_block(207-219)crates/storage/src/consensus_models/block_header.rs (1)
zero_block(199-221)
crates/template_lib/src/models/resource.rs (6)
crates/engine_types/src/published_template.rs (1)
as_object_key(59-61)crates/engine_types/src/transaction_receipt.rs (1)
as_object_key(56-58)crates/engine_types/src/validator_fee.rs (1)
as_object_key(47-49)crates/template_lib/src/models/claimed_output_tombstone.rs (1)
as_object_key(38-40)crates/template_lib/src/models/component.rs (1)
as_object_key(51-53)crates/template_lib/src/models/vault.rs (1)
as_object_key(82-84)
crates/template_lib/src/models/component.rs (6)
crates/engine_types/src/published_template.rs (1)
as_object_key(59-61)crates/engine_types/src/transaction_receipt.rs (1)
as_object_key(56-58)crates/engine_types/src/validator_fee.rs (1)
as_object_key(47-49)crates/template_lib/src/models/claimed_output_tombstone.rs (1)
as_object_key(38-40)crates/template_lib/src/models/resource.rs (1)
as_object_key(47-49)crates/template_lib/src/models/vault.rs (1)
as_object_key(82-84)
crates/consensus/src/validations/common.rs (3)
crates/consensus/src/validations/block.rs (2)
check_proposal_certificate(66-66)check_timeout_certificate(67-67)crates/consensus/src/hotstuff/commit_proofs.rs (1)
qc(234-251)crates/consensus/src/hotstuff/on_receive_new_view.rs (1)
check_quorum_certificate_signatures(187-187)
crates/consensus/src/traits/certificate.rs (5)
bindings/src/types/ProposalCertificate.ts (1)
ProposalCertificate(7-15)bindings/src/types/TimeoutCertificate.ts (1)
TimeoutCertificate(6-13)crates/consensus_types/src/bookkeeping/high_pc.rs (1)
height(43-45)crates/consensus_types/src/certificates/proposal_certificate.rs (1)
height(111-113)crates/consensus_types/src/certificates/quorum_certificate.rs (1)
height(61-66)
crates/transaction/src/v1/transaction.rs (1)
crates/engine_types/src/substate.rs (1)
fmt(471-483)
clients/tari_indexer_client/src/rest_api_client.rs (2)
crates/engine_types/src/events.rs (1)
template_address(107-109)bindings/src/types/tari-indexer-client/GetTemplateDefinitionResponse.ts (1)
GetTemplateDefinitionResponse(4-4)
crates/consensus_tests/src/support/transaction_executor.rs (3)
bindings/src/types/SubstateLockType.ts (1)
SubstateLockType(6-6)bindings/src/helpers/consts.ts (1)
XTR(10-10)bindings/src/types/SubstateId.ts (1)
SubstateId(6-6)
crates/consensus/src/hotstuff/vote_collector/collector.rs (5)
crates/template_lib_types/src/crypto/ristretto.rs (1)
zero(27-29)crates/template_lib_types/src/crypto/scalar.rs (1)
zero(24-26)crates/template_lib_types/src/crypto/schnorr.rs (2)
zero(22-27)signature(64-66)bindings/src/types/Epoch.ts (1)
Epoch(3-3)bindings/src/types/NodeHeight.ts (1)
NodeHeight(3-3)
crates/state_store_tests/src/blocks.rs (2)
crates/storage/src/consensus_models/block.rs (3)
has_justify_qc(400-402)is_committed(408-410)zero_block(207-219)crates/storage/src/consensus_models/block_header.rs (1)
zero_block(199-221)
crates/consensus_tests/src/support/validator/builder.rs (4)
crates/state_store_rocksdb/src/options.rs (1)
default(33-39)applications/tari_app_utilities/src/genesis_resources.rs (1)
get_stealth_tari_resource(30-54)crates/common_types/src/versioned_substate_id.rs (8)
substate_id(42-44)substate_id(222-224)substate_id(317-319)substate_id(442-444)new(21-26)new(193-195)new(306-311)new(431-433)crates/storage/src/consensus_models/substate.rs (1)
commit_batch(170-176)
crates/state_store_rocksdb/src/reader.rs (2)
crates/storage/src/state_store/mod.rs (3)
proposal_certificates_get(205-205)proposal_certificates_get_many(206-209)parked_block_exists(328-328)crates/state_store_rocksdb/src/cf_api.rs (1)
exists(130-137)
crates/consensus/src/hotstuff/pacemaker.rs (2)
crates/consensus/src/hotstuff/worker.rs (1)
on_leader_timeout(648-679)crates/consensus/src/hotstuff/on_leader_timeout.rs (1)
leader_timed_out(46-48)
crates/consensus/src/hotstuff/on_catch_up_sync.rs (2)
crates/consensus_types/src/bookkeeping/high_pc.rs (1)
epoch(51-53)crates/consensus/src/messages/message.rs (1)
epoch(66-78)
crates/wallet/sdk/src/apis/stealth_transfer/api.rs (2)
bindings/src/types/wallet-daemon-client/BadgeUsage.ts (1)
BadgeUsage(6-10)crates/wallet/sdk/src/apis/accounts.rs (2)
derive_account_address_from_public_key(62-64)derive_account_address_from_public_key(83-85)
applications/tari_walletd/src/services/template_monitor.rs (1)
crates/engine/src/wasm/module.rs (1)
load_template_from_code(65-100)
crates/consensus/src/hotstuff/on_inbound_message.rs (4)
crates/consensus_types/src/certificates/quorum_certificate.rs (2)
epoch(54-59)height(61-66)crates/storage/src/consensus_models/block.rs (2)
epoch(352-354)height(348-350)crates/storage/src/consensus_models/block_header.rs (2)
epoch(391-393)height(387-389)crates/consensus/src/messages/message.rs (1)
epoch(66-78)
crates/consensus/src/hotstuff/common.rs (2)
bindings/src/types/ProposalCertificate.ts (1)
ProposalCertificate(7-15)crates/storage/src/consensus_models/block_header.rs (1)
justify_id(383-385)
utilities/tariswap_test_bench/src/accounts.rs (1)
utilities/tariswap_test_bench/src/timer.rs (1)
info(32-34)
crates/consensus/src/validations/block.rs (1)
crates/consensus/src/validations/common.rs (2)
check_proposal_certificate(203-219)check_timeout_certificate(221-239)
crates/state_store_tests/src/state_tree_diff.rs (4)
crates/consensus_types/src/certificates/proposal_certificate.rs (1)
genesis(56-66)crates/storage/src/consensus_models/block.rs (1)
genesis(173-204)crates/storage/src/consensus_models/block_header.rs (1)
genesis(167-195)crates/state_store_tests/src/helpers.rs (1)
create_block(268-302)
crates/consensus_types/src/certificates/proposal_certificate.rs (4)
crates/consensus_types/src/certificates/quorum_certificate.rs (1)
calculate_id(40-45)crates/storage/src/consensus_models/block.rs (2)
calculate_id(221-223)shard_group(356-358)crates/storage/src/consensus_models/block_header.rs (2)
calculate_id(259-275)shard_group(395-397)bindings/src/types/ShardGroup.ts (1)
ShardGroup(4-4)
crates/consensus_types/src/certificates/quorum_certificate.rs (4)
bindings/src/types/ProposalCertificate.ts (1)
ProposalCertificate(7-15)bindings/src/types/TimeoutCertificate.ts (1)
TimeoutCertificate(6-13)crates/consensus_types/src/certificates/proposal_certificate.rs (6)
justifies_zero_block(95-97)signatures(107-109)calculate_id(71-76)epoch(99-101)height(111-113)fmt(151-161)crates/p2p/src/conversions/consensus.rs (15)
from(91-116)from(155-161)from(182-188)from(209-214)from(235-239)from(254-260)from(277-282)from(297-326)from(358-367)from(383-391)from(413-420)from(442-449)from(470-486)from(565-572)from(602-608)
crates/p2p/src/conversions/consensus.rs (2)
crates/storage/src/consensus_models/block_header.rs (1)
justify_id(383-385)crates/consensus_types/src/ids/mod.rs (2)
from(86-88)from(92-94)
crates/consensus/src/hotstuff/on_leader_timeout.rs (1)
crates/consensus/src/hotstuff/pacemaker_handle.rs (1)
new(39-53)
crates/state_store_rocksdb/src/writer.rs (3)
crates/storage/src/consensus_models/block.rs (2)
commit_qc_id(457-459)justify_qc_id(404-406)crates/state_store_rocksdb/src/cf_api.rs (1)
cf(32-41)crates/storage/src/state_store/mod.rs (1)
diagnostics_add_no_vote(579-579)
crates/wallet/sdk/src/apis/transaction.rs (3)
crates/engine_types/src/substate.rs (1)
component(590-595)bindings/src/types/IndexedWellKnownTypes.ts (1)
IndexedWellKnownTypes(15-30)crates/template_lib/src/models/vault.rs (2)
vault_id(144-149)vault_id(467-469)
crates/template_lib/src/models/claimed_output_tombstone.rs (6)
crates/engine_types/src/substate.rs (11)
new(71-76)from_bytes(98-100)from_bytes(197-199)from_bytes(787-789)try_from(375-383)try_from(389-397)try_from(403-411)try_from(417-425)try_from(431-439)try_from(445-453)try_from(459-467)crates/engine_types/src/published_template.rs (2)
from_hex(51-53)as_object_key(59-61)crates/engine_types/src/transaction_receipt.rs (3)
from_hex(60-62)from_array(51-54)as_object_key(56-58)crates/engine_types/src/validator_fee.rs (4)
from_hex(55-57)from_array(42-45)as_object_key(47-49)try_from(79-87)crates/template_lib/src/models/component.rs (4)
from_hex(61-64)from_array(67-69)as_object_key(51-53)as_bytes(56-58)crates/template_lib_types/src/crypto/commitment.rs (4)
from_hex(59-62)from_array(42-44)from_bytes(46-57)as_bytes(64-66)
crates/consensus/src/hotstuff/on_ready_to_vote_on_local_block.rs (3)
crates/storage/src/consensus_models/block.rs (3)
parent(323-325)header(225-227)commit_qc_id(457-459)crates/storage/src/consensus_models/block_header.rs (2)
parent(379-381)total_accumulated_exhaust_burn(439-441)bindings/src/types/Block.ts (1)
Block(7-35)
crates/template_lib_types/src/crypto/scalar.rs (4)
crates/template_lib_types/src/crypto/commitment.rs (1)
zero(34-36)crates/template_lib_types/src/crypto/ristretto.rs (1)
zero(27-29)crates/template_lib_types/src/crypto/schnorr.rs (1)
zero(22-27)crates/engine_types/src/hash.rs (1)
zero(29-31)
crates/template_lib_types/src/crypto/commitment.rs (3)
crates/template_lib_types/src/entity_id.rs (6)
from_array(33-35)from_array(153-155)as_bytes(29-31)as_bytes(126-128)into_array(37-39)into_array(157-159)crates/template_lib_types/src/hash.rs (2)
from_array(50-52)into_array(54-56)crates/template_lib_types/src/crypto/ristretto.rs (4)
length(23-25)as_bytes(49-51)into_array(57-59)as_hash(61-63)
crates/template_lib_types/src/crypto/schnorr.rs (4)
crates/template_lib_types/src/crypto/ristretto.rs (1)
zero(27-29)crates/template_lib_types/src/crypto/scalar.rs (1)
zero(24-26)crates/common_types/src/substate_address.rs (1)
zero(89-91)crates/template_lib_types/src/crypto/commitment_signature.rs (1)
zero(20-26)
crates/consensus/src/hotstuff/worker.rs (3)
crates/consensus/src/hotstuff/on_leader_timeout.rs (1)
wait(40-44)crates/consensus/src/hotstuff/on_inbound_message.rs (1)
next_message(35-57)crates/consensus_types/src/ids/mod.rs (1)
as_ref(68-73)
crates/template_lib_types/src/crypto/ristretto.rs (3)
crates/template_lib_types/src/crypto/commitment.rs (5)
zero(34-36)as_bytes(64-66)into_array(68-70)length(30-32)as_hash(72-74)crates/template_lib_types/src/crypto/schnorr.rs (2)
zero(22-27)length(18-20)crates/template_lib_types/src/hash.rs (1)
into_array(54-56)
crates/consensus_types/src/ids/mod.rs (1)
crates/consensus_types/src/certificates/quorum_certificate.rs (5)
is_proposal_certificate(18-20)is_timeout_certificate(22-24)fmt(98-103)from(86-88)from(92-94)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test
- GitHub Check: check stable
- GitHub Check: check nightly
- GitHub Check: machete
- GitHub Check: clippy
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
applications/tari_validator_node/log4rs_sample.yml (1)
1-7: ** Update file header to reflect new logging destinations.**The file header still describes the old three-destination logging split, but with the addition of consensus-related loggers now routing to
consensus.log, the documentation is out of sync. The header should be updated to mention the newconsensus.logalongside the existing destinations.Apply this diff to update the header:
# A sample log configuration file for running in release mode. By default, this configuration splits up log messages to -# three destinations: +# multiple destinations: # * Console: For log messages with level INFO and higher # * log/validator-node/network.log: INFO-level logs related to the comms crate. This file will be quite busy since there # are lots of P2P debug messages, and so this traffic is segregated from the application log messages -# * log/validator-node/ootle.log: Non-comms related INFO-level messages and higher are logged into this file +# * log/validator-node/ootle.log: Application-level INFO-level messages and higher +# * log/validator-node/consensus.log: Consensus and storage-related DEBUG-level messages # * log/validator-node/other.log: Third-party crates' messages will be logged here at an ERROR level
🧹 Nitpick comments (8)
applications/tari_validator_node/log4rs_sample.yml (1)
58-59: Fix copy-paste errors in appender comment headers.Lines 58 and 75 both reference "An appender named 'ootle'" but should reference 'engine' and 'consensus' respectively.
Apply this diff to correct the comments:
- # An appender named "ootle" that writes to a file with a custom pattern encoder + # An appender named "engine" that writes to a file with a custom pattern encoder engine: ... - # An appender named "ootle" that writes to a file with a custom pattern encoder + # An appender named "consensus" that writes to a file with a custom pattern encoder consensus:Also applies to: 75-76
applications/tari_validator_node/src/transaction_validators/basic.rs (1)
34-34: Clarify what additional checks are planned.The TODO comment is vague about what additional validation checks should be added here. Given the PR description mentions making XTR resource implicit to all transactions, is this TODO related to that change, or are there other specific validations planned?
applications/tari_validator_node/src/transaction_validators/network.rs (1)
90-101: Consider verifying error field values in the test.The test validates that a
NetworkMismatcherror is returned but doesn't verify that theactualandexpectedfields contain the correct values. Enhancing the test to check these fields would catch semantic errors like the one flagged above.Apply this diff to strengthen the test:
#[test] fn network_mismatch() { let network_byte = Network::MainNet.as_byte(); let validator = TransactionNetworkValidator::new(Network::LocalNet); let tx = tx(network_byte); let result = validator.validate(&(), &tx); assert!(result.is_err()); - assert!(matches!( - result.err().unwrap(), - TransactionValidationError::NetworkMismatch { actual: _, expected: _ }, - )); + match result.err().unwrap() { + TransactionValidationError::NetworkMismatch { actual, expected } => { + assert_eq!(actual, Network::MainNet, "actual should be the transaction's network"); + assert_eq!(expected, Network::LocalNet, "expected should be the validator's network"); + }, + _ => panic!("Expected NetworkMismatch error"), + } }crates/consensus/src/hotstuff/worker.rs (5)
101-101: First-block tracking now derives from LeafBlock height; confirm behaviour across epoch changes.Initializing
has_processed_first_blockfromLeafBlock::heightinstead of theis_genesisflag fixes the “genesis exists but no height‑1 block committed” restart case and is nicely wired throughworker_stateintonext_messageand toggled totrueon successful/duplicate proposal handling. However,has_processed_first_blockis only initialized instart()and then monotonically set totrue; it is never reset when the pacemaker epoch changes. If a singleHotstuffWorkerinstance runs across multiple epochs, this would mean the first‑block gating inmessage_buffer.next(.., has_processed_first_block)only ever protects the very first epoch—later epochs could again accept height>1 while the epoch’s first block is parked, recreating the described issue but for subsequent epochs. If the worker is reused across epochs, consider recomputinghas_processed_first_blockwheneverepoch_state.epochchanges (e.g., from the new epoch’sLeafBlockheight) so the protection applies per‑epoch rather than once per process.Also applies to: 227-227, 253-260, 369-369, 1053-1056, 1228-1232
428-429: Catch-up batching state machine looks sound but “done” threshold is slightly subtle.Threading
current_heightintohandle_hotstuff_errorfrom bothon_unvalidated_messageandon_proposal_messagegives the newCatchUpstate enough context to batch progress, and theWorkerState.catch_up+is_catching_up()guard prevent the previous catch-up/error/catch-up loop. TheCatchUplogic (initialexpected_batch_height = current_height + 99, advancing viaset_next_batch, and clearingcatch_upwhenset_next_batchreturnsfalse) will treat catch-up as “finished” once we’re within one batch ofhigh_qc(sinceexpected_batch_heightis derived from the latestcurrent_height), not necessarily whencurrent_height >= high_qc. That may be intentional (stop explicit catch-up once we’re close and let normal consensus finish), but if the intention is to ensure we actually reachhigh_qcbefore clearingcatch_up, you might wantset_next_batchto advance from the priorexpected_batch_heightinstead of fromcurrent_height. Also, note that only theFallenBehind/JustifyBlockNotFoundpath setsWorkerState.catch_up, so theis_catching_up()guard inhandle_hotstuff_erroronly suppresses additional sync in that class of errors and not for pure timeout-driven syncs.Also applies to: 482-483, 1019-1050, 1072-1072, 1111-1117, 1143-1150, 1240-1250
313-315: Timeout-triggered catch-up sync behaviour is reasonable; consider whether repeated probes are desired.Startup now always issues a single
request_catch_up_sync, andon_leader_timeouttriggers another sync every third timeout when not already “catching up”, which should help unstick nodes that stop seeing progress. BecauseWorkerState.catch_upis only populated fromhandle_hotstuff_errorand not whenrequest_catch_up_syncis called fromon_leader_timeout, pure timeout-driven syncs won’t flipis_catching_up()totrue, so multiple groups of three timeouts can result in several overlapping catch-up requests to committee peers. IfOnCatchUpSync::request_syncis cheap and idempotent this is fine; if not, you might consider tracking a lightweight “sync probe in flight” flag (or reusingWorkerState.catch_up) for the timeout path to avoid redundant requests while a previous sync is still active.Also applies to: 387-389, 602-615, 651-675
16-17: Propose gating on catch-up state and LastSentVote should prevent redundant or conflicting proposals.The early return in
propose_nowwhenis_catching_up()is true, followed by theLastSentVoteheight check, cleanly prevents proposing while we’re in a structured catch-up and also avoids proposing at or below a height for which we’ve already cast a vote, which matches HotStuff safety requirements. If you ever find this path hot, you could micro‑optimize by fetchingLastSentVotein the same read transaction asHighestSeenBlock/LastProposedused inon_beat/on_force_beat, but functionally this looks correct.Also applies to: 837-861
1160-1160: Genesis creation correctly switches to PcId, but the boolean result is unused.Using
PcId::zero()for both the zero block and genesis justify QC is consistent with the move toHighPcas the canonical justification type, and the write‑path (justify → insert → mark as locked/leaf/highest/last_* → commit) still looks coherent. The function now returnsResult<bool, _>but the caller instart()ignores the boolean, so from a reader’s perspective theboolreturn is a bit misleading. Since this is private, consider changing the signature toResult<(), HotStuffError>(or using theboolto drive a small log like “genesis already existed vs created”) to avoid confusion for future maintainers.Also applies to: 1169-1169, 1191-1191, 1200-1201
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
applications/tari_indexer/src/network_client.rs(0 hunks)applications/tari_indexer/src/rest_api/handlers/transactions.rs(0 hunks)applications/tari_validator_node/log4rs_sample.yml(1 hunks)applications/tari_validator_node/src/bootstrap.rs(3 hunks)applications/tari_validator_node/src/p2p/services/mempool/service.rs(0 hunks)applications/tari_validator_node/src/transaction_validators/basic.rs(1 hunks)applications/tari_validator_node/src/transaction_validators/error.rs(1 hunks)applications/tari_validator_node/src/transaction_validators/fee.rs(0 hunks)applications/tari_validator_node/src/transaction_validators/mod.rs(1 hunks)applications/tari_validator_node/src/transaction_validators/network.rs(1 hunks)applications/tari_walletd/src/services/template_monitor.rs(3 hunks)crates/consensus/src/hotstuff/worker.rs(20 hunks)crates/transaction/src/transaction.rs(0 hunks)utilities/tariswap_test_bench/src/tariswap.rs(2 hunks)
💤 Files with no reviewable changes (5)
- applications/tari_indexer/src/rest_api/handlers/transactions.rs
- crates/transaction/src/transaction.rs
- applications/tari_indexer/src/network_client.rs
- applications/tari_validator_node/src/p2p/services/mempool/service.rs
- applications/tari_validator_node/src/transaction_validators/fee.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- utilities/tariswap_test_bench/src/tariswap.rs
🧰 Additional context used
🧬 Code graph analysis (5)
applications/tari_validator_node/src/transaction_validators/network.rs (2)
crates/common_types/src/network.rs (1)
try_from(97-107)crates/transaction/src/transaction.rs (1)
network(81-85)
applications/tari_validator_node/src/bootstrap.rs (3)
crates/state_store_rocksdb/src/store.rs (1)
open(150-168)applications/tari_validator_node/src/config.rs (2)
default(142-171)default(190-194)applications/tari_validator_node/src/p2p/services/mempool/service.rs (1)
new(71-92)
crates/consensus/src/hotstuff/worker.rs (4)
crates/consensus/src/hotstuff/on_leader_timeout.rs (2)
default(52-54)wait(40-44)crates/consensus_types/src/bookkeeping/leaf_block.rs (2)
height(43-45)epoch(55-57)crates/consensus/src/hotstuff/common.rs (1)
get_highest_seen_justified_view(421-434)crates/consensus/src/hotstuff/on_inbound_message.rs (1)
next_message(35-57)
applications/tari_validator_node/src/transaction_validators/error.rs (1)
bindings/src/types/TransactionId.ts (1)
TransactionId(3-3)
applications/tari_walletd/src/services/template_monitor.rs (1)
crates/engine/src/wasm/module.rs (1)
load_template_from_code(65-100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: machete
- GitHub Check: test
- GitHub Check: check stable
- GitHub Check: clippy
- GitHub Check: check nightly
🔇 Additional comments (8)
applications/tari_validator_node/src/transaction_validators/mod.rs (1)
4-12: LGTM!The module reorganization consolidates validation logic appropriately by adding the
basicmodule and removing the separatefeeandis_shard_applicablemodules.applications/tari_validator_node/src/bootstrap.rs (2)
216-220: Verify the implications of unconditionally enabling debugging data.The
with_debugging_data(true)flag is enabled by default for all environments with a TODO comment indicating it should be configurable. This change could have storage or performance implications that should be assessed.Please verify:
- What data does the
debugging_dataflag persist, and what is the storage overhead?- Are there any performance impacts from enabling this flag in production?
- Should this be feature-gated or environment-specific (e.g., only enabled for testnets)?
Given that the PR description mentions persisting noVote diagnostic data, this change appears related. Consider documenting the rationale and impact in the code comments.
451-451: LGTM!The replacement of the old validation chain with
BasicValidations::new()correctly consolidates the previousIsShardApplicableand fee validation logic into a single validator.applications/tari_validator_node/src/transaction_validators/error.rs (1)
22-23: LGTM!The
NoFeeInstructionsvariant now correctly includes thetransaction_idfield, which improves error diagnostics by identifying which transaction failed validation.applications/tari_validator_node/src/transaction_validators/network.rs (1)
27-32: LGTM! Clean refactor of network extraction.The logic correctly extracts the network byte from the transaction and handles the conversion error by mapping it to
TransactionValidationError::UnknownNetworkwith full context (byte value and error details).applications/tari_walletd/src/services/template_monitor.rs (2)
5-10: LGTM! Imports align with the new on-chain template loading approach.The added imports support the shift from network-based fetching to direct WASM module loading via
spawn_blocking.
33-55: LGTM! Solid defensive checks and clear error reporting.The template extraction logic properly validates the substate type before attempting to load, and the existence check prevents unnecessary duplicate work.
crates/consensus/src/hotstuff/worker.rs (1)
16-19: Imports for new consensus types look consistent with usage.
LastSentVote,LeafBlock,PcId,Displayable,Optional, andLeaderTimeoutare all exercised later in the file (propose gating, genesis creation, logging, and catch-up), and the imports are scoped appropriately; I don’t see any issues here.Also applies to: 23-23, 56-56
| @@ -8,30 +8,32 @@ use crate::{transaction_validators::TransactionValidationError, validator::Valid | |||
|
|
|||
| const LOG_TARGET: &str = "tari::ootle::mempool::validators::is_shard_applicable"; | |||
There was a problem hiding this comment.
Update the LOG_TARGET constant to reflect the renamed struct.
The LOG_TARGET still references "is_shard_applicable" but the struct has been renamed to BasicValidations. This inconsistency could make debugging confusing.
Apply this diff:
-const LOG_TARGET: &str = "tari::ootle::mempool::validators::is_shard_applicable";
+const LOG_TARGET: &str = "tari::ootle::mempool::validators::basic_validations";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const LOG_TARGET: &str = "tari::ootle::mempool::validators::is_shard_applicable"; | |
| const LOG_TARGET: &str = "tari::ootle::mempool::validators::basic_validations"; |
🤖 Prompt for AI Agents
In applications/tari_validator_node/src/transaction_validators/basic.rs around
line 9, the LOG_TARGET constant still references
"tari::ootle::mempool::validators::is_shard_applicable" which is inconsistent
with the renamed struct BasicValidations; change the LOG_TARGET value to a name
that matches the struct (for example
"tari::ootle::mempool::validators::basic_validations") so logs reflect the
correct component and reduce confusion when debugging.
| if tx_network != self.network { | ||
| warn!(target: LOG_TARGET, "TransactionNetworkValidator - FAIL: mismatching networks: TX: {} != Current: {}", tx_network, self.network); | ||
| return Err(Self::Error::NetworkMismatch { | ||
| actual: self.network, | ||
| expected: tx_network, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Fix swapped actual and expected fields in error construction.
The actual and expected fields in the NetworkMismatch error are semantically reversed:
tx_networkis what was provided in the transaction (actual)self.networkis what the validator expects (expected)
The warning message confirms this interpretation: "TX: {} != Current: {}" where TX is the actual network and Current is the expected network.
Apply this diff to correct the field assignment:
if tx_network != self.network {
warn!(target: LOG_TARGET, "TransactionNetworkValidator - FAIL: mismatching networks: TX: {} != Current: {}", tx_network, self.network);
return Err(Self::Error::NetworkMismatch {
- actual: self.network,
- expected: tx_network,
+ actual: tx_network,
+ expected: self.network,
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if tx_network != self.network { | |
| warn!(target: LOG_TARGET, "TransactionNetworkValidator - FAIL: mismatching networks: TX: {} != Current: {}", tx_network, self.network); | |
| return Err(Self::Error::NetworkMismatch { | |
| actual: self.network, | |
| expected: tx_network, | |
| }); | |
| } | |
| if tx_network != self.network { | |
| warn!(target: LOG_TARGET, "TransactionNetworkValidator - FAIL: mismatching networks: TX: {} != Current: {}", tx_network, self.network); | |
| return Err(Self::Error::NetworkMismatch { | |
| actual: tx_network, | |
| expected: self.network, | |
| }); | |
| } |
🤖 Prompt for AI Agents
In applications/tari_validator_node/src/transaction_validators/network.rs around
lines 34 to 40, the NetworkMismatch error constructs its fields reversed: swap
the values so actual = tx_network and expected = self.network; update the
Err(...) to pass actual: tx_network, expected: self.network to match the warning
message and semantics.
2 participants
Description
fix(consensus): more robust catch-up sync
fix catch up -> error -> catch up loop when first block is parked
Persist noVote data for diagnostics
XTR resource is now implicit to all transactions
Motivation and Context
At the start of the epoch, blocks are normally produced as follows:
Initially, the pacemaker height is 0, and remains 0 after receiving block 1. If block 1 contains an unknown transaction, it is parked. During catch up, when many blocks are buffered, this would cause block 2 to be processed immediately, since we have not yet saved block 1 (since it is parked/unprocessed) justified in QC 1, we attempt to catch up.
How Has This Been Tested?
New catch up unit test, manually
What process can a PR reviewer use to test or verify this change?
Breaking Changes
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Enhancements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.