feat: auto zero value coinbase reward calculation#7259
Conversation
WalkthroughThis change updates the base node's gRPC server to handle cases where a block is created with a single coinbase of zero value, setting its value to a minimum or full block reward as appropriate. Two integration test steps are added to verify block creation and submission with a zero-value coinbase. A new test scenario validates this behavior via gRPC. Changes
Sequence Diagram(s)sequenceDiagram
participant TestStep as Integration Test Step
participant Node as Tari Node (gRPC)
TestStep->>Node: Request new block template with coinbases (zero value)
Node->>Node: If single coinbase & value == 0, set to block reward (min or full)
Node-->>TestStep: Return block with adjusted coinbase
TestStep->>Node: Submit block
Node-->>TestStep: Block submission result
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
🧹 Nitpick comments (4)
applications/minotari_node/src/grpc/base_node_grpc_server.rs (1)
1388-1392: Consider standardizing the reward conversion approach.The auto-coinbase logic is correct, but there's an inconsistency with the first method where
reward as u64is used versusreward.as_u64()here. While both work correctly with their respective types, consider using a consistent pattern for clarity.- if coinbases.len() == 1 && coinbases[0].value == 0 { - coinbases[0].value = reward.as_u64(); - } + if coinbases.len() == 1 && coinbases[0].value == 0 { + coinbases[0].value = reward.as_u64(); + }This maintains the current functionality while keeping the conversion method consistent with the established pattern in this codebase.
integration_tests/tests/steps/node_steps.rs (3)
1013-1064: Remove debug print statement and consider using test constants for addresses.The test logic is correct for validating the zero coinbase feature, but there are code quality improvements needed:
- The
println!statement on line 1051 should be removed for production code- The hardcoded wallet address reduces maintainability
- println!("{}", body);Consider extracting the hardcoded address to a test constant:
+const TEST_WALLET_ADDRESS: &str = "f4L8GRWsXqz26DM3qAGErLtVknYzmTe2fYP2yKFn4biFXYJMP61W9MeD726QJ7ytWhRGyewTZzTzjZ7tEPskDptwRub"; ... - address: TariAddress::from_base58( - "f4L8GRWsXqz26DM3qAGErLtVknYzmTe2fYP2yKFn4biFXYJMP61W9MeD726QJ7ytWhRGyewTZzTzjZ7tEPskDptwRub", - ) - .unwrap() - .to_base58(), + address: TariAddress::from_base58(TEST_WALLET_ADDRESS) + .unwrap() + .to_base58(),
1081-1081: Remove unused variable.The
amountvariable is calculated but never used, which appears to be leftover code from a copy-paste operation.- let amount = miner_data.reward + miner_data.total_fees;
1066-1122: Consider refactoring to reduce code duplication.This function shares significant code with
generate_block_as_single_request_with_zero_coinbase, including:
- Hardcoded wallet address
- Identical validation logic for coinbase counts
- Identical block submission logic
Consider extracting common functionality:
const TEST_WALLET_ADDRESS: &str = "f4L8GRWsXqz26DM3qAGErLtVknYzmTe2fYP2yKFn4biFXYJMP61W9MeD726QJ7ytWhRGyewTZzTzjZ7tEPskDptwRub"; fn create_zero_coinbase() -> NewBlockCoinbase { NewBlockCoinbase { address: TariAddress::from_base58(TEST_WALLET_ADDRESS) .unwrap() .to_base58(), value: 0, stealth_payment: false, revealed_value_proof: true, coinbase_extra: Vec::new(), } } fn validate_and_submit_zero_coinbase_block( client: &mut grpc::base_node_client::BaseNodeClient<tonic::transport::Channel>, block: grpc::Block, ) { let mut coinbase_kernel_count = 0; let mut coinbase_utxo_count = 0; let body: AggregateBody = block.body.clone().unwrap().try_into().unwrap(); for kernel in body.kernels() { if kernel.is_coinbase() { coinbase_kernel_count += 1; } } for utxo in body.outputs() { if utxo.is_coinbase() { coinbase_utxo_count += 1; } } assert_eq!(coinbase_kernel_count, 1); assert_eq!(coinbase_utxo_count, 1); match client.submit_block(block).await { Ok(_) => (), Err(e) => panic!("The block should have been valid, {}", e), } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
applications/minotari_node/src/grpc/base_node_grpc_server.rs(3 hunks)integration_tests/tests/steps/node_steps.rs(1 hunks)
🔇 Additional comments (1)
applications/minotari_node/src/grpc/base_node_grpc_server.rs (1)
1126-1130: LGTM! Auto-coinbase calculation implementation looks correct.The logic appropriately handles the single zero-value coinbase case by setting it to the full block reward. The conditional check ensures this only applies when there's exactly one coinbase with zero value, which aligns with the PR objectives for mining pool convenience.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
integration_tests/tests/features/BlockTemplate.feature (1)
21-21: Fix typo in scenario name.There's a spelling error in the scenario name: "gprc" should be "grpc".
Apply this diff to fix the typo:
- Scenario: Verify gprc can create block with zero value coinbase + Scenario: Verify grpc can create block with zero value coinbase
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
integration_tests/tests/features/BlockTemplate.feature(1 hunks)
🔇 Additional comments (1)
integration_tests/tests/features/BlockTemplate.feature (1)
20-26: Well-structured test scenario for zero value coinbase functionality.The test scenario follows the established patterns in the file and appropriately tests both the separate request and single request approaches. The scenario is correctly marked as critical and maintains consistency with existing test naming conventions.
MoneroOcean
changed the title
Co-authored-by: stringhandler <stringhandler@gmail.com>
Test Results (CI) 3 files 126 suites 42m 49s ⏱️ Results for commit 05a6fb4. ♻️ This comment has been updated with latest results. |
Test Results (Integration tests) 2 files + 2 10 suites +10 1h 9m 4s ⏱️ + 1h 9m 4s For more details on these failures, see this check. Results for commit cea06f7. ± Comparison against base commit 86539c8. |
hansieodendaal
left a comment
hansieodendaal
left a comment
There was a problem hiding this comment.
Please run
cargo +nightly-2025-01-01 fmt --all
otherwise looking good!
* development: chore: fix regression bug (tari-project#7276) feat!: expand gRPC readiness status to contain current processed block info (tari-project#7262) fix!: payref migration and indexes, add grpc query via output hash (tari-project#7266) feat: auto zero value coinbase reward calculation (tari-project#7259) feat!: improve grpc token supply (tari-project#7261) chore: new release v4.7.0-pre.0 (tari-project#7268) fix: get_all_completed_transactions limit issues (tari-project#7267) fix: ledger builds (tari-project#7260) feat: offline signing (tari-project#7122) test: verify accumulated difficulty (tari-project#7243)
metalaureate
added
bounty
bounty-S
retro-bounty
5 participants
Description
This adds way to auto use correct (max) coinbase value in case you pass single coinbase with zero value.
Motivation and Context
This is hard to guess correct coinbase value to pass its check in GetNewBlockWithCoinbases and GetNewBlockTemplateWithCoinbases GRPC methods.
How Has This Been Tested?
Tested as part of RandomX-T pool implementation. Will add new integration tests to this PR but I was not able to run them locally since I can't find seed node setup environment.
What process can a PR reviewer use to test or verify this change?
Run new integration scenario: Verify gprc can create block with zero value coinbase
Breaking Changes
Summary by CodeRabbit