fix(consensus)!: mutate validator fee substate using deposit/withdrawal #1313
Conversation
WalkthroughThe changes span multiple modules and components, refactoring how fee amounts and fee withdrawal data are accessed and handled. Direct field accesses were replaced with method calls across CLI and daemon commands. Import paths were simplified, redundant methods removed, and new helper methods and types (such as for validator fee withdrawals) added. Consensus, engine, and storage components were also refactored to incorporate fee withdrawal support, along with corresponding updates in the test harness and module reorganization. Changes
Sequence Diagram(s)sequenceDiagram
participant TX as Transaction
participant OP as OnPropose
participant PS as PendingStore
participant WS as WorkingState
participant SD as SubstateDiff
TX->>OP: Receive transaction
OP->>OP: Execute local_prepare_transaction(tx)
OP->>PS: Call update_in_place(substate_id, updater, creator)
PS->>WS: Trigger withdraw_all_fees_from_pool(address)
WS->>WS: Record fee withdrawal details
WS->>SD: Generate substate diff (include fee_withdrawals)
SD-->>OP: Return updated block diff
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
dan_layer/consensus/src/hotstuff/common.rs (1)
370-410: 🛠️ Refactor suggestionGracefully handle invalid leader fee values without panicking.
The protective assert for a negative or zerototal_leader_feemight cause a crash if any unexpected input arises during production. Consider returning an error or early exit instead of an assert to preserve system stability. Also, returning an error fromdeposit_direct(rather than a boolean) could help identify and handle deposit failures more precisely.- assert!( - total_leader_fee.is_positive(), - ... - ); - if total_leader_fee.is_zero() { ... } + if total_leader_fee <= Amount::zero() { + return Err(HotStuffError::InvariantError(format!( + "Leader fee must be greater than 0, got {}", + total_leader_fee + ))); + }
🧹 Nitpick comments (16)
dan_layer/common_types/src/shard_group.rs (2)
94-99: Add documentation for the new methodThe implementation of
contains_or_globalis clear and follows a logical pattern, but it lacks inline documentation. Consider adding a doc comment to explain the purpose and behavior of this method, similar to other methods in this file.+ /// Returns true if the shard is global or is contained within this shard group's range. pub fn contains_or_global(&self, shard: &Shard) -> bool { if shard.is_global() { return true; } self.contains(shard) }
94-99: Add unit tests for new methodThe new
contains_or_globalmethod should have unit tests to ensure it works correctly for all scenarios: global shards, contained shards, and non-contained shards.Consider adding a test like this to the test module:
#[test] fn contains_or_global_works_correctly() { let sg = ShardGroup::new(10, 20); // Test with a global shard let global_shard = Shard::global(); // Assuming this constructor exists assert!(sg.contains_or_global(&global_shard)); // Test with a contained shard let contained_shard = Shard::from(15); assert!(sg.contains_or_global(&contained_shard)); // Test with a non-contained shard let non_contained_shard = Shard::from(30); assert!(!sg.contains_or_global(&non_contained_shard)); }utilities/tariswap_test_bench/src/templates.rs (1)
21-21: Improved template name matching robustness.Switching from exact case matching to case-insensitive matching improves the robustness of template retrieval. This change helps prevent issues where the template name might have different capitalization than expected.
dan_layer/storage/src/consensus_models/substate_change.rs (2)
16-17: TODO comment regarding transaction_id field.Consider addressing this TODO as part of this PR or creating a follow-up task to evaluate if this field is necessary.
23-24: TODO comment regarding transaction_id field in Down variant.Same TODO appears in both variants - consider addressing both TODOs together for consistency.
dan_layer/consensus_tests/src/support/transaction.rs (3)
39-39: Confirm if a borrowed type might simplify usage.
The function signature now takesvalidator_fee_withdrawals: Vec<ValidatorFeeWithdrawal>. If the call sites only need read access, consider accepting a slice reference (&[ValidatorFeeWithdrawal]) to avoid unnecessary cloning. Otherwise, this usage is fine.
90-99: Avoid hardcoded large constant in tests unless absolutely needed.
Theamount: 100_000.into()is likely only for test scaffolding. Keeping it hardcoded is acceptable for quick testing, but allowing a configurable test constant might be more flexible for expanded test coverage.
102-103: Revise panic message for naming consistency.
The message references “vn fee” but the struct is namedValidatorFeePool. For clarity, consider adjusting the wording to “validator fee pool output.”- "create_execution_result_for_transaction: Test harness only supports generating component, vn fee, and template outputs." + "create_execution_result_for_transaction: Test harness only supports generating component, validator fee pool, and template outputs."dan_layer/consensus_tests/src/support/epoch_manager.rs (1)
95-122: Fluent interface pattern adoption.
Returning&Selffromadd_committeesenhances chained usage. This is a neat approach, but consider adding unit tests to ensure method chaining works as intended.dan_layer/engine/src/runtime/working_state.rs (3)
85-86: Field addition forvalidator_fee_withdrawals.
Storing attempted or completed fee withdrawals directly inWorkingStateimproves traceability of validator fee flows. Ensure that any concurrency or multi-transaction scenarios are well-handled.Also applies to: 118-119
770-770: Minor rename in error context.
Renaming the function reference toStateTracker::withdraw_all_fees_from_poolcan help pinpoint the correct scope in logs or debug traces.
775-778: Recording the withdrawal.
Appending theValidatorFeeWithdrawalto an internal vector is simple and robust. It might be beneficial to also log these events for operational visibility.dan_layer/engine_types/src/validator_fee.rs (1)
124-156: New direct deposit/withdraw methods.
These methods extend the fee pool’s functionality. The checks for negative or insufficient amounts are correct. If you anticipate large integer calculations, consider additional checks for overflow beyondchecked_addandchecked_sub_positive.dan_layer/engine_types/src/substate.rs (1)
872-881: Single-Set Method for Fee Withdrawals
The assertion helps ensure this field is set exactly once. However, an assertion will panic at runtime if the field is unexpectedly set again.Consider handling this scenario more gracefully (e.g., returning an error) instead of panicking. For instance:
pub fn set_once_fee_withdrawals(&mut self, withdrawals: Vec<ValidatorFeeWithdrawal>) -> Result<&mut Self, SomeError> { - assert!(self.fee_withdrawals.is_empty(), "Fee withdrawals set more than once"); + if !self.fee_withdrawals.is_empty() { + return Err(SomeError::AlreadySet); + } self.fee_withdrawals = withdrawals; Ok(self) }dan_layer/consensus/src/hotstuff/substate_store/pending_store.rs (1)
94-172: In-Place Substate Update
Addingupdate_in_placesimplifies or centralizes modifications to an existing substate. This approach helps consolidate creation vs. update logic.To improve resilience, consider providing partial rollback or explicit error handling for the scenario where
FUpdatefails after partial modifications, ensuring no inconsistent states are left in memory.dan_layer/consensus/src/hotstuff/on_propose.rs (1)
741-829: Commented-Out End-Epoch Helper
This large block is currently commented out.Consider removing or annotating why this code is kept commented. Dead code can lead to confusion or maintenance overhead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
bindings/dist/helpers/helpers.d.tsis excluded by!**/dist/**bindings/dist/index.d.tsis excluded by!**/dist/**bindings/dist/index.jsis excluded by!**/dist/**bindings/dist/types/SubstateDiff.d.tsis excluded by!**/dist/**bindings/dist/types/ValidatorFeeWithdrawal.d.tsis excluded by!**/dist/**bindings/dist/types/ValidatorFeeWithdrawal.jsis excluded by!**/dist/**bindings/dist/types/validator-node-client/ValidatorNodeChange.d.tsis excluded by!**/dist/**
📒 Files selected for processing (53)
applications/tari_dan_wallet_cli/src/command/transaction.rs(1 hunks)applications/tari_dan_wallet_daemon/src/handlers/validator.rs(1 hunks)applications/tari_validator_node/src/p2p/services/mempool/service.rs(2 hunks)applications/tari_validator_node_cli/src/command/transaction.rs(1 hunks)bindings/src/helpers/helpers.ts(1 hunks)bindings/src/index.ts(1 hunks)bindings/src/types/SubstateDiff.ts(1 hunks)bindings/src/types/ValidatorFeeWithdrawal.ts(1 hunks)bindings/src/types/validator-node-client/ValidatorNodeChange.ts(1 hunks)clients/wallet_daemon_client/src/types.rs(1 hunks)dan_layer/common_types/src/committee.rs(2 hunks)dan_layer/common_types/src/fee_pool.rs(1 hunks)dan_layer/common_types/src/shard_group.rs(1 hunks)dan_layer/common_types/src/substate_type.rs(2 hunks)dan_layer/common_types/src/versioned_substate_id.rs(1 hunks)dan_layer/consensus/src/hotstuff/block_change_set.rs(1 hunks)dan_layer/consensus/src/hotstuff/common.rs(3 hunks)dan_layer/consensus/src/hotstuff/on_propose.rs(9 hunks)dan_layer/consensus/src/hotstuff/on_ready_to_vote_on_local_block.rs(1 hunks)dan_layer/consensus/src/hotstuff/substate_store/error.rs(1 hunks)dan_layer/consensus/src/hotstuff/substate_store/pending_store.rs(9 hunks)dan_layer/consensus/src/hotstuff/transaction_manager/manager.rs(3 hunks)dan_layer/consensus_tests/src/consensus.rs(11 hunks)dan_layer/consensus_tests/src/support/epoch_manager.rs(5 hunks)dan_layer/consensus_tests/src/support/executions_store.rs(2 hunks)dan_layer/consensus_tests/src/support/harness.rs(9 hunks)dan_layer/consensus_tests/src/support/transaction.rs(4 hunks)dan_layer/consensus_tests/src/support/transaction_executor.rs(2 hunks)dan_layer/consensus_tests/src/support/validator/builder.rs(4 hunks)dan_layer/engine/src/runtime/impl.rs(2 hunks)dan_layer/engine/src/runtime/mod.rs(1 hunks)dan_layer/engine/src/runtime/tracker.rs(1 hunks)dan_layer/engine/src/runtime/working_state.rs(7 hunks)dan_layer/engine_types/src/indexed_value.rs(1 hunks)dan_layer/engine_types/src/instruction.rs(1 hunks)dan_layer/engine_types/src/lib.rs(1 hunks)dan_layer/engine_types/src/substate.rs(6 hunks)dan_layer/engine_types/src/substate_serde.rs(1 hunks)dan_layer/engine_types/src/validator_fee.rs(4 hunks)dan_layer/state_store_sqlite/migrations/2023-06-08-091819_create_state_store/up.sql(1 hunks)dan_layer/state_store_sqlite/src/schema.rs(2 hunks)dan_layer/static_epoch_oracle/Cargo.toml(0 hunks)dan_layer/static_epoch_oracle/src/config.rs(0 hunks)dan_layer/static_epoch_oracle/src/lib.rs(0 hunks)dan_layer/static_epoch_oracle/src/oracle.rs(0 hunks)dan_layer/storage/src/consensus_models/block.rs(2 hunks)dan_layer/storage/src/consensus_models/block_diff.rs(2 hunks)dan_layer/storage/src/consensus_models/lock_intent.rs(1 hunks)dan_layer/storage/src/consensus_models/substate_change.rs(2 hunks)dan_layer/template_lib/src/models/amount.rs(3 hunks)dan_layer/transaction/src/builder.rs(1 hunks)utilities/tariswap_test_bench/src/runner.rs(1 hunks)utilities/tariswap_test_bench/src/templates.rs(1 hunks)
💤 Files with no reviewable changes (4)
- dan_layer/static_epoch_oracle/src/config.rs
- dan_layer/static_epoch_oracle/src/lib.rs
- dan_layer/static_epoch_oracle/Cargo.toml
- dan_layer/static_epoch_oracle/src/oracle.rs
✅ Files skipped from review due to trivial changes (12)
- dan_layer/engine_types/src/indexed_value.rs
- dan_layer/common_types/src/fee_pool.rs
- dan_layer/engine/src/runtime/mod.rs
- dan_layer/transaction/src/builder.rs
- clients/wallet_daemon_client/src/types.rs
- utilities/tariswap_test_bench/src/runner.rs
- bindings/src/types/ValidatorFeeWithdrawal.ts
- dan_layer/engine_types/src/instruction.rs
- dan_layer/state_store_sqlite/migrations/2023-06-08-091819_create_state_store/up.sql
- dan_layer/consensus/src/hotstuff/block_change_set.rs
- dan_layer/engine_types/src/substate_serde.rs
- dan_layer/consensus/src/hotstuff/on_ready_to_vote_on_local_block.rs
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test
- GitHub Check: clippy
- GitHub Check: check nightly
- GitHub Check: check stable
🔇 Additional comments (101)
applications/tari_dan_wallet_cli/src/command/transaction.rs (1)
550-550: Method call change aligns with validator fee pool encapsulation pattern.The change from direct field access to method call (
pool.amount()) is part of a broader refactoring effort around validator fee pools, providing better encapsulation and allowing for additional logic in the getter method.bindings/src/types/SubstateDiff.ts (1)
4-4: New fee withdrawal tracking supports the validator fee management improvements.The addition of the
ValidatorFeeWithdrawalimport and the newfee_withdrawalsproperty to theSubstateDiffinterface aligns with the PR's objective to improve validator fee substate management, particularly for tracking withdrawals from claim fee transactions.Also applies to: 9-9
dan_layer/common_types/src/versioned_substate_id.rs (1)
258-262: API improvement with AsRef implementation.This new implementation allows
SubstateRequirementRefto be used with functions that acceptAsRef<SubstateId>, making the API more flexible and consistent with Rust's standard patterns.bindings/src/index.ts (1)
123-123: Exported ValidatorFeeWithdrawal completes the public API.Exporting the
ValidatorFeeWithdrawaltype makes it available to consumers of the library, which is necessary since it's now used in theSubstateDiffinterface.bindings/src/types/validator-node-client/ValidatorNodeChange.ts (2)
3-3: New import added for SubstateAddress.The import of SubstateAddress has been added to support the new field in the ValidatorNodeChange type.
6-6: Added shard_key field to ValidatorNodeChange Add operation.The ValidatorNodeChange.Add operation now includes a shard_key field of type SubstateAddress, which enables better identification of the validator node's location within the sharding structure. This change aligns with the PR objective to improve validator fee management across shards.
dan_layer/storage/src/consensus_models/lock_intent.rs (1)
142-146: Good addition of AsRef trait implementation.Implementing
AsRef<SubstateId>forVersionedSubstateIdLockIntentimproves interoperability with Rust components that expectAsReftraits. This follows Rust's best practices for enabling more ergonomic and generic code.The implementation correctly delegates to the existing
substate_id()method, which maintains code consistency and avoids duplication.dan_layer/state_store_sqlite/src/schema.rs (2)
545-553: New table added for tracking validator fee changes.The addition of the
validator_fee_changestable is a critical part of this PR's objective to better handle validator fee deposits and withdrawals. This table will store records of fee changes, enabling proper tracking and reconciliation of fee movements.The schema includes all necessary fields:
- Primary key (
id)- Block identifier (
block_id)- Fee pool address (
address)- Change amount (
amount)- Timestamp (
created_at)
607-607: Table properly registered for queries.The
validator_fee_changestable has been correctly added to theallow_tables_to_appear_in_same_query!macro, enabling it to be joined with other tables in queries.utilities/tariswap_test_bench/src/templates.rs (1)
28-28: Consistent case-insensitive matching for TestFaucet template.Using case-insensitive matching here as well ensures consistent behavior throughout the template matching logic.
applications/tari_dan_wallet_daemon/src/handlers/validator.rs (1)
92-92: Appropriate field access encapsulation.The change from direct field access to method call (
p.amount()instead of likelyp.amount) follows good object-oriented practices by encapsulating the internal representation of the validator fee pool. This provides flexibility for future implementation changes without modifying the API contract.dan_layer/consensus_tests/src/support/executions_store.rs (2)
11-11: LGTM: Appropriate import update.The import has been correctly updated to include
ValidatorFeeWithdrawalfor the new field being added.
45-45: Fee withdrawals field added to support the PR objectives.This new field aligns with the PR description of collecting validator withdrawals within a block and consolidating them into a single state change. This ensures proper tracking of fee withdrawals during test executions, which is essential for validating the new behavior.
dan_layer/template_lib/src/models/amount.rs (3)
23-23: Import correctly updated for Neg trait.The import has been updated to include the
Negtrait which is implemented below.
35-35: Serialization support added conditionally.The addition of
borsh::BorshSerializesupport is properly guarded by a feature flag, maintaining backward compatibility.
163-169:❓ Verification inconclusive
Neg trait implementation enables crucial fee withdrawal functionality.
This implementation supports the PR's key objective of handling validator fee withdrawals by allowing
Amountinstances to be negated. This is essential for representing withdrawals as negative amounts, which simplifies the math operations when consolidating fee deposits and withdrawals.Consider adding tests to verify the negation behavior works as expected.
🏁 Script executed:
#!/bin/bash # Check if tests for Amount negation exist rg -A 2 -B 2 "fn.*neg" --glob "*.rs" | grep -i "test"Length of output: 53
Neg trait implementation is vital for the fee withdrawal logic, but no tests currently verify its behavior.
The implementation in
dan_layer/template_lib/src/models/amount.rs(lines 163-169) cleanly inverts the underlying value, fulfilling the PR's objective for handling validator fee withdrawals. However, our searches did not reveal any tests that verify this behavior. Please add comprehensive tests to confirm that negating anAmountconsistently produces the expected negative value.dan_layer/common_types/src/substate_type.rs (2)
7-7: LGTM: Import correctly updated.The import has been updated to include
Substatewhich is used in the new implementation below.
59-63: Useful convenience implementation for type conversion.This implementation provides a direct conversion from
&SubstatetoSubstateType, allowing for more concise code when determining substate types. It appropriately leverages the existing conversion fromSubstateValuetoSubstateType.This supports the PR objective by making it easier to work with validator fee pool substates in the updated consensus logic.
dan_layer/consensus/src/hotstuff/substate_store/error.rs (1)
22-23: New error variant looks good for invariant violations.This addition provides a clear way to report invariant errors with specific details, which is valuable for debugging and error handling.
dan_layer/engine_types/src/lib.rs (1)
37-39: Good refactoring of validator fee module.The reorganization from
vn_fee_pooltovalidator_feemakes sense, and the public re-export ensures backward compatibility.applications/tari_validator_node_cli/src/command/transaction.rs (1)
390-391: Improved encapsulation by replacing field access with method calls.Changing from direct field access to method calls is a good practice for better encapsulation and supports the PR's objectives for fee pool management.
dan_layer/engine/src/runtime/impl.rs (2)
47-47: Import update aligns with module reorganization.Updated import path for
ValidatorFeePoolAddressis consistent with the module changes in engine_types.
2243-2244: Simplified return statement.Minor code improvement that maintains the same functionality while making the code cleaner.
dan_layer/engine/src/runtime/tracker.rs (1)
305-306:✅ Verification successful
Integrate validator fee withdrawals in substate diff generation.
This change adds support for collecting validator fee withdrawals in the finalize process, passing them to the
generate_substate_diffmethod. This aligns with the PR objective of consolidating validator fee updates into a single state change.
🏁 Script executed:
#!/bin/bash # Check how fee_withdrawals is used in related files rg -A 5 -B 5 "generate_substate_diff.*fee_withdrawals" --type rustLength of output: 1039
Integration Confirmed: Validator Fee Withdrawals in Substate Diff Generation
The updated code correctly captures validator fee withdrawals and passes them to
generate_substate_diff. The shell script output confirms that the following changes are present indan_layer/engine/src/runtime/tracker.rs:
- Retrieving the fee withdrawals with
state.take_validator_fee_withdrawals()- Passing these withdrawals into
state.generate_substate_diffalong with the transaction receipt and substates.This implementation meets the PR objective by consolidating validator fee updates into a single state change process.
bindings/src/helpers/helpers.ts (1)
76-80: Expanded the function signature to accept string-type substate IDs.The function now accepts string substate IDs in addition to SubstateId objects, making it more flexible for various call sites.
dan_layer/consensus_tests/src/support/validator/builder.rs (4)
36-36: Added fee claim public key field to ValidatorBuilder.This new field supports the validator fee claim functionality, allowing validators to authenticate when claiming fees.
52-52: Initialized fee_claim_public_key with default value.Properly initializes the new field with a default value in the constructor.
81-84: Added method to set custom fee claim public key.This builder method follows the same pattern as other setter methods in the class, allowing custom configuration of the fee claim public key.
127-127: Pass fee claim public key to epoch manager.Ensures the fee claim public key is properly passed to the epoch manager when spawning a validator.
dan_layer/storage/src/consensus_models/substate_change.rs (2)
79-84:❓ Verification inconclusive
Added mutable access to substates.
This new method replaces the previous
down()method with one that provides mutable access to the substate in the Up variant. This is a significant change in functionality that supports the PR objective of managing validator fee substates when locked.
🏁 Script executed:
#!/bin/bash # Check for any remaining usages of the old down method rg "\.down\(\)" --type rust # Check how the new up_mut method is used rg "\.up_mut\(\)" --type rustLength of output: 61
Action: Confirm the replacement of
down()withup_mut()and manually verify dependent code usage.
- The new method
up_mut()correctly provides mutable access to the substate in theUpvariant.- A repository-wide search did not find any remaining calls to the old
down()method, and no obvious usages ofup_mut()were detected.- Please manually verify that no downstream code expects the previous behavior of
down()and that the transition toup_mut()aligns with the PR’s objective of managing validator fee substates when locked.
72-77:✅ Verification successful
Renamed method for better clarity.
The method rename from
uptoup_substatemakes the purpose clearer and helps distinguish from the newup_mutmethod.
🏁 Script executed:
#!/bin/bash # Check for any remaining usages of the old method name rg "\.up\(\)" --type rustLength of output: 27
Method rename verified. The renaming from
uptoup_substateclearly enhances code clarity, and our verification did not find any residual usages of the old.up()method in the repository.
- Verified file:
dan_layer/storage/src/consensus_models/substate_change.rsshows the correct implementation ofup_substate.- No instances of
.up()were found throughout the codebase.applications/tari_validator_node/src/p2p/services/mempool/service.rs (2)
23-23: Import simplificationThe removal of the
itermodule from the imports is a clean simplification, as it's no longer needed with the updated implementation at line 251-255.
251-255: Code simplification and improved efficiencyChanged from using
iter::once()withincludes_any_addressto directly usingincludes_substate_address. This is a more direct and efficient approach that avoids creating an intermediate iterator for a single item.dan_layer/consensus/src/hotstuff/transaction_manager/manager.rs (2)
199-199: API improvement for local input checksReplaced specific address checking with the more efficient
is_all_localmethod that works directly with transaction inputs. This aligns with the PR's goal of improving fee substate handling by streamlining the approach to determining transaction locality.
255-255: API improvement for output locality checksSimilar to the change at line 199, this replaces complex address-based checking with a direct
is_all_localcall on transaction outputs. This provides a more consistent and efficient approach to determining if a transaction's outputs are all within the local shard.dan_layer/storage/src/consensus_models/block_diff.rs (3)
39-39: Parameter name clarificationRenamed parameter from
infotocommittee, improving code readability by more accurately reflecting the parameter's purpose and type.
45-45: Improved filter logic for substatesChanged from checking individual substate IDs to checking if the committee's shard group contains or is global to the change's shard. This is a significant improvement that aligns with the PR's goal of better handling validator fee substates across shards.
The new approach focuses on the shard relationship rather than individual substates, which should provide more accurate filtering when processing validator fee deposits and withdrawals that span multiple shards.
64-64: Method name simplificationRenamed method from
insert_recordtoinsert, making the API more concise while maintaining its functionality. This is a good cleanup that follows naming conventions of similar operations.dan_layer/storage/src/consensus_models/block.rs (2)
641-645: Enhanced type flexibility with generic constraintsAdded clearer generic constraints to
commit_diffrequiring both write capabilities and the ability to dereference to a read transaction. This enhancement supports the PR's goal of properly managing validator fee substates by ensuring the method has appropriate access for both reading current state and writing consolidated changes.
669-671: Cleaner block diff destructuringImproved the code by directly destructuring
block_diffinto its components, making the subsequent iteration over changes cleaner and more direct.dan_layer/consensus_tests/src/support/transaction.rs (2)
17-18: Imports look consistent with new validator fee functionality.
These imports are correctly introduced and align with the introduced usage ofValidatorFeePoolandValidatorFeeWithdrawal.
123-123: Ensure no double-writing of fee withdrawals.
Callingset_once_fee_withdrawalsis correct, but verify that no other code attempts to override these withdrawals later. The “once” naming suggests it should only be called once; confirm the logical flow so it’s never inadvertently called multiple times, resulting in an overwrite or partial application.dan_layer/consensus_tests/src/support/transaction_executor.rs (2)
110-117: Handle unmatched validator fee withdrawals explicitly.
The.filter_map(...)approach silently skips any withdrawal with no matching input. If those withdrawals should never be missing a corresponding input, consider raising an error instead of ignoring them, to detect misconfigurations early.
126-126: Signature change properly passes validator fee withdrawals.
Forwardingspec.validator_fee_withdrawalstocreate_execution_result_for_transactionis well-aligned with the new logic. Ensure test coverage validates this pipeline fully.dan_layer/consensus/src/hotstuff/common.rs (3)
43-43: New import aligns with substate and fee functionality.
Importingsubstate::SubstateDiff,template_models::Amount, andValidatorFeePoolis consistent with the changes below.
51-51: ImportingLeaderStrategyis properly aligned with usage.
This import is consistent with references to leader-based logic introduced in this file.
313-332: Filter fee withdrawals by committee.
Infilter_diff_for_committee, callingset_once_fee_withdrawalsto include only relevant validator fee withdrawals enhances consistency with how up/down items are filtered. Confirm that partial filtering of withdrawals is intended when multiple committees are involved.dan_layer/consensus_tests/src/consensus.rs (7)
15-18: New imports look good.They are used for the new functionality (e.g.,
to_public_key_bytes), and no issues or unused imports are apparent.
21-22: Relevant imports for fee pool logic and key pair generation.These are clearly utilized in the new fee-claim test scenario, and the usage appears consistent.
41-41: ValidatorFeeWithdrawal import is consistent.The import aligns with the new fee-withdrawal logic, ensuring the struct is accessible for test usage.
919-919: Added validator_fee_withdrawals field in ExecuteSpec initialization.Setting an empty vector here is appropriate for transactions that do not claim fees.
927-927: Repeated addition of validator_fee_withdrawals.Similar usage; no issues. Ensures future fee withdrawals can be handled without modifying existing transaction logic.
1189-1189: Validator fee withdrawals for single_shard_unversioned_inputs.Maintaining a default empty vector avoids unexpected side effects in tests that do not involve fee claims.
1597-1684: New multishard_validator_fee_claim test.This test correctly demonstrates claiming validator fees from a cross-committee scenario. However, you may wish to:
- Verify the final deducted balance of the fee pool to ensure the withdrawal is reflected, thus fully confirming the claim logic.
Would you like a script to inspect the final substate value post-claim and confirm the deducted balance?
dan_layer/common_types/src/committee.rs (2)
4-4: Imports for RangeInclusive are valid.They support the substate address range method; usage is consistent.
257-264: Local substate check logic.The method
is_all_localcorrectly handles global substates in multi-committee contexts by returning false. This meets the intended design to restrict “local” to only those substates mapped to this committee.dan_layer/consensus_tests/src/support/harness.rs (6)
14-14: New PublicKey import.Allows the harness to handle fee claim key injection without extraneous references.
221-224: Provides constant access to TEST_NUM_PRESHARDS.A neat approach for consistent test configuration.
225-227: num_committees getter.Clean accessor for the test’s committee count.
543-543: Optional claim_keys field on TestBuilder.Allows specifying a fee claim key and VN destination.
601-604: Builder method for claim key.Cleanly sets up the claim_keys tuple for test usage.
666-667: Fee claim public key injection in validator.Ensures the validator is aware of the claim key during startup.
dan_layer/consensus_tests/src/support/epoch_manager.rs (4)
20-25: Imports look clean and consistent.
No immediate concerns here; the introduction ofTestVnDestinationalongside the other utility imports appears correct and aligned with the new usage below.
65-71: Method signature enhancement.
Addingfee_claim_pkas a parameter is a logical extension. Ensure all call sites are adjusted to pass this new argument to avoid errors.Would you like a script to detect any references to
clone_forwithout the updated signature?
80-81: Correct usage offee_claim_public_key.
Settingfee_claim_public_keyin both branches ensures that the newly-clonedValidatorNodealways has the intended claim key. This is good for consistency.Also applies to: 89-90
123-132: Newset_claim_keysmethod.
The method is straightforward and maintains the chaining pattern. Confirm that callers properly handle scenarios where no validator nodes match the destination.dan_layer/engine/src/runtime/working_state.rs (5)
31-32: New imports for validator fee types.
ImportingValidatorFeePoolAddressandValidatorFeeWithdrawalis logical for the new fee-handling functionality.
753-764: Ownership check and usage.
This block correctly checks ownership for fee withdrawal (require_ownership). The pattern aligns with the existing approach to validating privileged operations. Good job ensuring that any direct substate references are consistent with engine-level checks.
838-840: Retrieving fee withdrawals.
Exposing atake_validator_fee_withdrawalsmethod neatly encapsulates the data handover. This approach avoids partial reads and helps maintain consistent state.
1136-1141: Substate diffusion of fee withdrawals.
Passingfee_withdrawalsintogenerate_substate_diffand setting them on the diff is a clean design choice, making the final SubstateDiff more comprehensive.
1142-1145: Downing existing substates before upserting new ones.
This approach is consistent with the usual "down then up" pattern for substate changes. Ensure any unrelated changed lines keep that pattern.dan_layer/engine_types/src/validator_fee.rs (6)
1-2: License header update.
The updated year aligns with the current timeline.
11-12: Serialization import.
Introducingserdefor serialization and deserialization is consistent with the rest of the codebase.
157-159: Addingamount()accessor.
Using a dedicated getter clarifies usage and preserves the invariants of the struct’s state field.
161-163: Getter for claim public key.
Good alignment with the typical Rust pattern of providing an owned or borrowed accessor.
165-165: Retain existingwithdraw_alllogic.
No changes needed, as the method still handles the complete removal of funds. The inline docstring reaffirms usage in the engine.
185-196: IntroducingValidatorFeeWithdrawalstruct.
A well-defined structure for capturing withdrawal events. This paves the way for robust tracking and auditing.dan_layer/engine_types/src/substate.rs (9)
57-59: Imports for Validator Fee Pool Components
These newly added imports properly align with the subsequent usage ofValidatorFeePool,ValidatorFeePoolAddress, andValidatorFeeWithdrawal.
181-186: Accessor for Validator Fee Pool Address
The newas_validator_fee_pool_addressfunction is consistent with the code pattern used for other substate variants. It provides a convenient way to retrieve theValidatorFeePoolAddress.
292-295: Checker for Validator Fee Pool IDs
Addingis_validator_fee_poolcomplements the existing checks for other substate types. It improves clarity when working with validator fee pool substates.
764-769: Converter to Validator Fee Pool
Theinto_validator_fee_poolmethod forSubstateValuemirrors the existing conversion patterns. Logic and naming appear consistent.
857-857: New Field for Fee Withdrawals
Thefee_withdrawalsvector inSubstateDiffallows consolidating validator fee withdrawal records, aligning with the larger fee management refactor.
861-861: Initialize Fee Withdrawals Vector
Initializingfee_withdrawalsto an empty vector follows the established pattern for newly added fields.
869-871: 'up' Method for SubstateDiff
Adding a separateupmethod for pushing substates is consistent and keeps the code organized. No concerns found.
888-889: 'down' Method for SubstateDiff
This straightforward helper adds a down-substate pair to the internal collection, consistent with the existing pattern.
909-912: Getter for Validator Fee Withdrawals
Returning a slice reference is memory-efficient and matches the rest of the API style.dan_layer/consensus/src/hotstuff/substate_store/pending_store.rs (8)
11-11: Import ofSubstateType
Including thesubstate_type::SubstateTypeimport appears to prepare usage for substate classification or debugging.
26-26: Engine Types Substate Imports
BringingSubstate,SubstateDiff,SubstateId, andSubstateValueinto scope is consistent with the changes that handle fee withdrawals and in-place updates.
67-92: Retrieving the Latest Change from Store
This helper function consolidates logic for obtaining the most recent up/down state from both the pending block diff and the underlying store. Looks logically sound.Please confirm that all relevant edge cases (like multiple block diffs, stale references, or concurrency issues) are handled correctly in higher layers.
183-189: Handling an 'Up' Substate inget
Returning the cloned substate if present is consistent with typical up-substate logic.
232-234: Skipping Validator Fee Pools for Down Iteration
if id.is_validator_fee_pool() { continue; }ensures that fee pool substates are treated separately from standard up/down logic, matching the new approach.
245-249: Skipping Validator Fee Pools for Up Iteration
Similarly bypassing fee pool updates here avoids standard substate flow, aligning with specialized handling for validator fees.
261-295: Applying Validator Fee Withdrawals
The in-place update callsvalue_mut.as_validator_fee_pool_mut()and checks for balance sufficiency, ensuring correct invariants. Good defensive coding.
695-696: Exposingget_latest_lock_by_idPublicly
Making this method public allows external code to retrieve lock details for a substate when necessary. The logic inside remains consistent.dan_layer/consensus/src/hotstuff/on_propose.rs (7)
419-419: Addingcan_propose_epoch_endParameter
Renaming or adding this parameter clarifies when the code can produce an epoch-ending block rather than everyday transitions.
434-443: Conditional Logic for Epoch-End vs. Normal Batch
Reading fromcan_propose_epoch_endensures correct branching for either standard transaction proposals or an end-of-epoch scenario.
446-449: InstantiatingPendingSubstateStore
Setting up the store with parent block ID and preshards is consistent with the new fee management approach.
452-461: Commands for Epoch End
Whencan_propose_epoch_endistrue, it immediately inserts theEndEpochcommand. This approach is straightforward and clear.
1212-1223: Utility for Non-Local Shards
Collecting shards not belonging to the local committee is essential for foreign proposal logic. Straightforward approach.
1231-1231: NewcommandsField inProposalBatch
Storingcommandsalongside foreign proposals, burnt UTXOs, etc., aligns with the block-building flow.
1239-1243: Extended Display Output
Including the number of newcommandsin thefmtoutput improves logging and observability of the batch’s contents.
Test Results (CI)17 tests 16 ✅ 1m 1s ⏱️ For more details on these failures, see this check. Results for commit ec82ad2. ♻️ This comment has been updated with latest results. |
sdbondi
force-pushed
the
consensus-validator-fees-fix
branch
from
769038c to
3441d23
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
dan_layer/common_types/src/shard_group.rs(2 hunks)dan_layer/consensus/src/hotstuff/block_change_set.rs(2 hunks)dan_layer/consensus/src/hotstuff/on_propose.rs(9 hunks)dan_layer/storage/src/consensus_models/substate_change.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- dan_layer/consensus/src/hotstuff/block_change_set.rs
- dan_layer/consensus/src/hotstuff/on_propose.rs
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test
- GitHub Check: check stable
- GitHub Check: clippy
- GitHub Check: check nightly
🔇 Additional comments (3)
dan_layer/common_types/src/shard_group.rs (2)
94-99: LGTM - Clear implementation of special case handling for global shardsThe
contains_or_globalmethod elegantly handles the special case of global shards while still leveraging the existingcontainsmethod for standard shard checks. The method name clearly describes its purpose and the implementation follows the single responsibility principle.
204-219: Good test coverage for all scenariosThe test properly validates all three important scenarios for the
contains_or_globalmethod:
- Global shard returns true (regardless of range)
- Contained shard returns true
- Non-contained shard returns false
This ensures the method works correctly across all expected use cases.
dan_layer/storage/src/consensus_models/substate_change.rs (1)
16-17:❓ Verification inconclusive
Consider investigating these TODO comments about
transaction_idfields.There are TODO comments questioning whether the
transaction_idfields are needed in both theUpandDownvariants. These fields are currently used in thetransaction_id()method and in theDisplayimplementation. If they're not needed elsewhere, consider removing them to reduce memory overhead and simplify the data structure.Also applies to: 23-24
🏁 Script executed:
#!/bin/bash # Check other usages of transaction_id in SubstateChange across the codebase rg "SubstateChange.*transaction_id" --type rustLength of output: 315
Action Required: Evaluate necessity of the
transaction_idfield in both variants.After verification, the
transaction_idfield is used only in the two locations withinSubstateChange(in pattern matching for both theUpandDownvariants) as well as in thetransaction_id()method and in theDisplayimplementation. Please review if maintaining the field in both variants is justified. If it turns out that the field isn’t needed elsewhere (for example, if its value can be derived or managed more efficiently), consider removing it to minimize memory overhead and simplify the structure.
- File(s):
dan_layer/storage/src/consensus_models/substate_change.rs- Location: Lines 16-17 and 23-24
| pub fn up_substate(&self) -> Option<&Substate> { | ||
| match self { | ||
| SubstateChange::Up { substate, .. } => Some(substate), | ||
| _ => None, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider the redundancy between substate() and up_substate() methods.
The newly renamed method up_substate() appears to have identical functionality to the existing substate() method (lines 43-48). Both return Option<&Substate> for the Up variant and None for the Down variant. This redundancy could cause confusion for developers. Consider:
- Deprecating one of these methods
- Updating documentation to clarify the distinction between them
- Removing one if they are truly redundant
The renaming from up() to up_substate() improves clarity about what is being returned, but the duplication of functionality is concerning.
sdbondi
force-pushed
the
consensus-validator-fees-fix
branch
from
6c1f29f to
ec82ad2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
dan_layer/consensus_tests/src/support/epoch_manager.rs (1)
123-132: New set_claim_keys method for fee claim public key managementThis new method allows updating fee claim public keys for multiple validator nodes based on a destination criteria. It supports the PR objective of managing validator fee withdrawals.
Consider adding documentation to explain the purpose of this method and how it integrates with the broader validator fee management functionality. The method is well-designed to allow for method chaining with the return of
&Self.+/// Updates the fee claim public key for validator nodes matching the specified destination. +/// This allows for controlling which public key is authorized to claim fees for specific validators. +/// Returns a reference to self to enable method chaining. pub async fn set_claim_keys(&self, dest: TestVnDestination, claim_key: PublicKey) -> &Self {dan_layer/engine_types/src/validator_fee.rs (1)
1-2: Copyright year should be verified.The copyright year is set to 2025 - should this be the current year (2024) or is this intentional for future-dating?
-// Copyright 2025 The Tari Project +// Copyright 2024 The Tari Projectdan_layer/common_types/src/committee.rs (1)
257-265: Add documentation for the special case behavior with global substates.The implementation correctly checks if all provided substate IDs are local to this committee. However, there's a special case where global substates are considered "local" when there's only one committee (
num_committees <= 1).Consider adding a doc comment to clarify this behavior:
+ /// Checks if all provided substate IDs are local to this committee. + /// + /// Returns: + /// - `false` if any ID is global and there is more than one committee + /// - `false` if any ID is not included in this committee's shard range + /// - `true` otherwise (including the case where a global ID exists but there's only one committee) pub fn is_all_local<T: AsRef<SubstateId>, I: IntoIterator<Item = T>>(&self, substate_ids: I) -> bool { substate_ids.into_iter().all(|substate_id| { let substate_id = substate_id.as_ref(); if substate_id.is_global() && self.num_committees > 1 { return false; } self.includes_substate_id(substate_id) }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
bindings/dist/helpers/helpers.d.tsis excluded by!**/dist/**bindings/dist/index.d.tsis excluded by!**/dist/**bindings/dist/index.jsis excluded by!**/dist/**bindings/dist/types/SubstateDiff.d.tsis excluded by!**/dist/**bindings/dist/types/ValidatorFeeWithdrawal.d.tsis excluded by!**/dist/**bindings/dist/types/ValidatorFeeWithdrawal.jsis excluded by!**/dist/**bindings/dist/types/wallet-daemon-client/WebauthnFinishRegisterResponse.d.tsis excluded by!**/dist/**
📒 Files selected for processing (52)
applications/tari_dan_wallet_cli/src/command/transaction.rs(1 hunks)applications/tari_dan_wallet_daemon/src/handlers/validator.rs(1 hunks)applications/tari_validator_node/src/p2p/services/mempool/service.rs(2 hunks)applications/tari_validator_node_cli/src/command/transaction.rs(1 hunks)bindings/src/helpers/helpers.ts(1 hunks)bindings/src/index.ts(1 hunks)bindings/src/types/SubstateDiff.ts(1 hunks)bindings/src/types/ValidatorFeeWithdrawal.ts(1 hunks)bindings/src/types/wallet-daemon-client/WebauthnFinishRegisterResponse.ts(1 hunks)clients/wallet_daemon_client/src/types.rs(1 hunks)dan_layer/common_types/src/committee.rs(2 hunks)dan_layer/common_types/src/fee_pool.rs(1 hunks)dan_layer/common_types/src/shard_group.rs(2 hunks)dan_layer/common_types/src/substate_type.rs(2 hunks)dan_layer/common_types/src/versioned_substate_id.rs(1 hunks)dan_layer/consensus/src/hotstuff/block_change_set.rs(2 hunks)dan_layer/consensus/src/hotstuff/common.rs(3 hunks)dan_layer/consensus/src/hotstuff/on_propose.rs(9 hunks)dan_layer/consensus/src/hotstuff/on_ready_to_vote_on_local_block.rs(1 hunks)dan_layer/consensus/src/hotstuff/substate_store/error.rs(1 hunks)dan_layer/consensus/src/hotstuff/substate_store/pending_store.rs(9 hunks)dan_layer/consensus/src/hotstuff/transaction_manager/manager.rs(3 hunks)dan_layer/consensus_tests/src/consensus.rs(11 hunks)dan_layer/consensus_tests/src/support/epoch_manager.rs(5 hunks)dan_layer/consensus_tests/src/support/executions_store.rs(2 hunks)dan_layer/consensus_tests/src/support/harness.rs(9 hunks)dan_layer/consensus_tests/src/support/transaction.rs(4 hunks)dan_layer/consensus_tests/src/support/transaction_executor.rs(2 hunks)dan_layer/consensus_tests/src/support/validator/builder.rs(4 hunks)dan_layer/engine/src/runtime/impl.rs(2 hunks)dan_layer/engine/src/runtime/mod.rs(1 hunks)dan_layer/engine/src/runtime/tracker.rs(1 hunks)dan_layer/engine/src/runtime/working_state.rs(7 hunks)dan_layer/engine_types/src/indexed_value.rs(1 hunks)dan_layer/engine_types/src/instruction.rs(1 hunks)dan_layer/engine_types/src/lib.rs(1 hunks)dan_layer/engine_types/src/substate.rs(6 hunks)dan_layer/engine_types/src/substate_serde.rs(1 hunks)dan_layer/engine_types/src/validator_fee.rs(4 hunks)dan_layer/state_store_sqlite/migrations/2023-06-08-091819_create_state_store/up.sql(1 hunks)dan_layer/static_epoch_oracle/Cargo.toml(0 hunks)dan_layer/static_epoch_oracle/src/config.rs(0 hunks)dan_layer/static_epoch_oracle/src/lib.rs(0 hunks)dan_layer/static_epoch_oracle/src/oracle.rs(0 hunks)dan_layer/storage/src/consensus_models/block.rs(2 hunks)dan_layer/storage/src/consensus_models/block_diff.rs(2 hunks)dan_layer/storage/src/consensus_models/lock_intent.rs(1 hunks)dan_layer/storage/src/consensus_models/substate_change.rs(2 hunks)dan_layer/template_lib/src/models/amount.rs(3 hunks)dan_layer/transaction/src/builder.rs(1 hunks)utilities/tariswap_test_bench/src/runner.rs(1 hunks)utilities/tariswap_test_bench/src/templates.rs(1 hunks)
💤 Files with no reviewable changes (4)
- dan_layer/static_epoch_oracle/src/lib.rs
- dan_layer/static_epoch_oracle/src/config.rs
- dan_layer/static_epoch_oracle/Cargo.toml
- dan_layer/static_epoch_oracle/src/oracle.rs
🚧 Files skipped from review as they are similar to previous changes (39)
- applications/tari_dan_wallet_cli/src/command/transaction.rs
- dan_layer/storage/src/consensus_models/lock_intent.rs
- bindings/src/types/ValidatorFeeWithdrawal.ts
- bindings/src/index.ts
- utilities/tariswap_test_bench/src/runner.rs
- applications/tari_dan_wallet_daemon/src/handlers/validator.rs
- utilities/tariswap_test_bench/src/templates.rs
- dan_layer/consensus/src/hotstuff/substate_store/error.rs
- dan_layer/engine_types/src/indexed_value.rs
- dan_layer/engine/src/runtime/tracker.rs
- dan_layer/common_types/src/fee_pool.rs
- dan_layer/common_types/src/versioned_substate_id.rs
- applications/tari_validator_node/src/p2p/services/mempool/service.rs
- dan_layer/engine_types/src/substate_serde.rs
- dan_layer/transaction/src/builder.rs
- dan_layer/engine_types/src/lib.rs
- dan_layer/template_lib/src/models/amount.rs
- clients/wallet_daemon_client/src/types.rs
- dan_layer/storage/src/consensus_models/substate_change.rs
- dan_layer/engine/src/runtime/mod.rs
- applications/tari_validator_node_cli/src/command/transaction.rs
- dan_layer/consensus/src/hotstuff/transaction_manager/manager.rs
- dan_layer/consensus/src/hotstuff/on_ready_to_vote_on_local_block.rs
- dan_layer/consensus_tests/src/support/executions_store.rs
- dan_layer/consensus_tests/src/support/validator/builder.rs
- dan_layer/engine/src/runtime/impl.rs
- dan_layer/consensus_tests/src/support/transaction_executor.rs
- dan_layer/consensus_tests/src/support/transaction.rs
- dan_layer/storage/src/consensus_models/block_diff.rs
- dan_layer/consensus/src/hotstuff/block_change_set.rs
- dan_layer/consensus/src/hotstuff/common.rs
- bindings/src/helpers/helpers.ts
- dan_layer/state_store_sqlite/migrations/2023-06-08-091819_create_state_store/up.sql
- dan_layer/storage/src/consensus_models/block.rs
- dan_layer/consensus/src/hotstuff/on_propose.rs
- dan_layer/consensus/src/hotstuff/substate_store/pending_store.rs
- dan_layer/consensus_tests/src/support/harness.rs
- bindings/src/types/SubstateDiff.ts
- dan_layer/engine_types/src/instruction.rs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test
- GitHub Check: clippy
- GitHub Check: check nightly
🔇 Additional comments (39)
dan_layer/common_types/src/shard_group.rs (2)
94-99: Clear implementation of global or local shard check.The method provides a useful abstraction by combining the global shard check with the existing contains logic, which will help simplify code that needs to handle both global and local shards.
204-219: Comprehensive test coverage for the new method.The test effectively covers all three possible scenarios: global shard, contained shard, and non-contained shard, ensuring the method behaves correctly in all cases.
dan_layer/consensus_tests/src/consensus.rs (6)
15-18: Improved import organization.The import update from
tari_consensusdemonstrates better organization by importing specific modules rather than using the entire namespace.
21-22: Added support for key generation from seeds.This addition of
create_key_pair_from_seedis necessary for the new validator fee claim test where deterministic key generation is needed.
41-41: Added ValidatorFeeWithdrawal type.This import supports the new functionality for validator fee management, essential for the new withdrawal system.
919-920: Added validator fee withdrawals field.The new field in ExecuteSpec supports the PR objective of tracking fee withdrawals.
927-928: Consistently added validator fee withdrawals field.All instances of ExecuteSpec have been consistently updated with the new field, maintaining the structure's integrity across the codebase.
Also applies to: 1189-1190, 1273-1274, 1284-1285, 1374-1375, 1385-1386, 1560-1561
1597-1684: Comprehensive test implementation for validator fee claims.This new test is crucial for validating the PR's core functionality - handling validator fee withdrawals across multiple shards. The test:
- Sets up committees across multiple shards
- Creates a deterministic key pair for claiming
- Creates a fee pool address based on the claim key
- Builds and executes a claim transaction with appropriate withdrawals
- Verifies the transaction is committed and fee pool exists
The test effectively validates the entire lifecycle of validator fee withdrawals, which directly addresses the PR objective.
dan_layer/consensus_tests/src/support/epoch_manager.rs (3)
20-25: Import addition of TestVnDestinationThe codebase imports
TestVnDestinationwhich is now used in the newset_claim_keysmethod. This aligns with the PR objectives of managing validator fee withdrawals.
65-71: Breaking change to clone_for method signatureThe
clone_formethod now requires a newfee_claim_pkparameter which is used to set thefee_claim_public_keyfield of theValidatorNodestruct. This is a breaking change that supports the PR objective of updating validator fee substate management.Make sure all calling code has been updated to provide this additional parameter. This change is consistent with the PR description that mentions this is a breaking change requiring data directory deletion.
Also applies to: 80-80, 89-89
95-121: Modified add_committees to enable method chainingThe
add_committeesmethod now returns&Selfand has an explicit return statement, enabling method chaining for a more fluent API.dan_layer/engine/src/runtime/working_state.rs (7)
31-32: New imports look appropriate.The imports for
ValidatorFeePoolAddressandValidatorFeeWithdrawalare correctly added to support the new fee withdrawal tracking functionality.
85-85: Good addition of withdrawal tracking.New field to track validator fee withdrawals aligns well with the PR objective of managing validator fee substate (vnfp) effectively.
118-118: Proper initialization of the new field.The
validator_fee_withdrawalsvector is correctly initialized as empty in the constructor.
748-780: Authorization and withdrawal tracking implemented correctly.The function properly checks ownership before allowing withdrawal and tracks the withdrawn amount with the appropriate validator address. This change aligns with the PR objective of improving how withdrawals are handled when the vnfp substate is locked.
838-840: Efficient implementation of withdrawal collection.Using
mem::takeis a good pattern here as it efficiently replaces the vector with an empty one while returning the original contents, avoiding unnecessary allocations.
1132-1141: Updated to include fee withdrawals in substate diff.The method now correctly accepts fee withdrawals as a parameter and properly integrates them into the substate diff. This supports the PR objective of consolidating updates into a single state change.
1142-1155: Maintain correct handling of versioning and substates.The loop has been slightly restructured but maintains the same logic for handling versioning of substates, special cases for empty fee pools, and correctly upserting substates into the diff.
dan_layer/engine_types/src/validator_fee.rs (7)
11-12: Import simplification looks good.The imports have been simplified to use serde directly and tari_bor specifically for BorTag, which improves code clarity.
124-137: Direct withdrawal implementation is well-designed.The
withdraw_directmethod properly validates that the withdrawal amount is valid before updating the pool amount, usingchecked_sub_positiveto prevent underflow. The warning comment about not using this in the engine is important and the#[must_use]attribute ensures the caller checks the return value.
139-155: Direct deposit handles negative amounts correctly.The
deposit_directmethod properly validates the deposit amount, ensuring it's not negative before attempting to add it to the pool. It useschecked_addto prevent overflow and returns a boolean to indicate success, which is marked with#[must_use]to ensure callers check the result.
157-159: Accessor pattern implementation is good.Adding the
amount()method is a good practice to encapsulate direct field access, which will make future refactoring easier.
161-163: Consistent accessor pattern.The
claim_public_key()accessor method matches the accessor pattern foramount(), providing consistent API design.
165-168: Updated docstring provides clearer context.The updated docstring clarifies that
withdraw_allis used in the engine to withdraw funds and create a Bucket, which differentiates it from the newwithdraw_directmethod.
186-195: New withdrawal data structure appropriately designed.The
ValidatorFeeWithdrawalstruct is properly designed with serialization support and appropriate fields for address and amount. The TypeScript export configuration will ensure bindings are correctly generated.dan_layer/engine_types/src/substate.rs (10)
57-59: New imports are properly referenced.The new imports for
ValidatorFeePool,ValidatorFeePoolAddress, andValidatorFeeWithdrawalare correctly added to support the new functionality.
181-186: Accessor method implementation is complete.The
as_validator_fee_pool_addressmethod correctly handles extraction of the validator fee pool address from theSubstateIdenum, following the established pattern for other substate types.
292-294: Type checking method implementation is correct.The
is_validator_fee_poolmethod follows the same pattern as other type checking methods in the class, using the matches! macro for a clean implementation.
764-769: Conversion method added for API completeness.The
into_validator_fee_poolmethod correctly implements the conversion fromSubstateValuetoValidatorFeePool, following the same pattern as other conversion methods, which maintains API consistency.
857-857: New field added to track fee withdrawals.The
fee_withdrawalsfield is correctly added to theSubstateDiffstruct to track validator fee withdrawals, which is a key part of the PR objectives to consolidate fee updates.
865-865: Proper initialization of the new field.The
fee_withdrawalsvector is properly initialized as empty in the constructor, ensuring consistent behavior with other fields.
869-871: Method renaming improves clarity.Renaming the parameter from
addresstoidbetter reflects that it's aSubstateIdtype, which improves code readability and clarity.
874-881: Set-once method has appropriate safeguards.The
set_once_fee_withdrawalsmethod correctly includes an assertion to ensure it's only called once, which prevents accidental overwriting of withdrawal data. The return of&mut selfallows for method chaining.
888-889: Parameter renaming maintained for consistency.The
downmethod parameter was also renamed fromaddresstoidfor consistency with theupmethod, which is a good practice.
909-911: Accessor method for fee withdrawals is properly implemented.The
validator_fee_withdrawalsmethod correctly returns a slice reference to the fee withdrawals, allowing read-only access to the data.dan_layer/common_types/src/substate_type.rs (2)
7-7: Updated import to include the Substate type.The import statement has been updated to include
SubstatealongsideSubstateValue, which is needed for the newFromimplementation below.
59-63: LGTM: Added convenientFromimplementation for&Substate.This implementation elegantly converts from
&SubstatetoSubstateTypeby leveraging the existing conversion fromSubstateValue. This is a useful enhancement that makes it more convenient to work with substates in the codebase.dan_layer/common_types/src/committee.rs (2)
4-4: Updated imports for the new functionality.Added
RangeInclusiveto the imports, which is used in the existingto_substate_address_rangemethod.
257-265:✅ Verification successful
Verify this method's usage in transaction manager.
According to the PR summary, this change is part of fixing how validator fee substate changes are handled when locked by transactions. Let's verify that this method provides the necessary functionality for the transaction manager.
🏁 Script executed:
#!/bin/bash # Check how the new is_all_local method is used in the transaction manager rg -A 5 "is_all_local" dan_layer/consensus/src/hotstuff/transaction_manager/Length of output: 1758
Verification of
is_all_localUsage in Transaction ManagerThe usage of
is_all_localin the transaction manager appears to be correct. The method is invoked both on transaction inputs (viatransaction.transaction.all_inputs_iter()) and on resulting outputs (viaexecution.resulting_outputs()), ensuring that checks for local-only processing are appropriately applied. This aligns with the PR summary intent of correctly handling validator fee substate changes when transaction locks are involved.
- Location:
dan_layer/consensus/src/hotstuff/transaction_manager/manager.rs
- Confirmed usage on transaction inputs.
- Confirmed usage on transaction outputs.
- Outcome: The functionality provided by
is_all_localis effectively integrated into the transaction management logic, and no discrepancies were identified during verification.
| export interface WebauthnFinishRegisterResponse { | ||
| success: boolean; | ||
| } | ||
| export type WebauthnFinishRegisterResponse = Record<string, never>; |
There was a problem hiding this comment.
Breaking change: WebauthnFinishRegisterResponse no longer contains a success property
This change modifies WebauthnFinishRegisterResponse from an interface with a success: boolean property to an empty record type (Record<string, never>). This is a breaking change that will affect any client code that was previously accessing the success property.
Since this is auto-generated from Rust code via ts-rs, ensure that:
- Client TypeScript/JavaScript code has been updated to handle this new structure
- Consumers of this API are aware of the change
- This breaking change is mentioned in release notes or migration guides
Description
Updates the latest vnfp change as per the current chain according to withdrawal(s) from a claim fee transaction
Fixed a bug with determining shard involvement for a transaction to account for the global shard.
Motivation and Context
As a leader gains fees from a block, it DOWN->UP the vnfp substate. However this substate could be locked by a transaction. When this transaction is committed, the diff DOWNs the previously locked substate which is invalid.
This PR collects validator withdrawals and fee deposits within a block and, "flattens" these fee pool updates into one state change for the block. It uses the latest state change as per the current uncommitted chain and updates it according to a withdrawal. Only one transaction may lock the vnfp (as with any other write lock), since validators only increase the vnfp substate value, this guarantees that the amount locked is less than the amount available within the vnfp substate (will cause a crash if not).
How Has This Been Tested?
New unit test,
Manually - running a tariswap test while claiming fees which previously broke the VN
What process can a PR reviewer use to test or verify this change?
As above
Breaking Changes
Summary by CodeRabbit
New Features
Improvements
Chores