perf: remove keymanager from creating coinbases #6970
Conversation
WalkthroughThis set of changes refactors coinbase and block creation logic across several components by removing the dependency on asynchronous key manager interfaces and instead using direct cryptographic key handling with Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant CoinbaseBuilder
participant CryptoFactories
Caller->>CoinbaseBuilder: new() / with keys
Caller->>CoinbaseBuilder: build(crypto_factories, ...)
CoinbaseBuilder->>CryptoFactories: generate commitments, proofs, signatures
CryptoFactories-->>CoinbaseBuilder: cryptographic results
CoinbaseBuilder-->>Caller: Transaction, Output, Kernel, SecretKey
sequenceDiagram
participant Node
participant BaseNodeGrpcServer
participant CryptoFactories
Node->>BaseNodeGrpcServer: get_new_block_template_with_coinbases
BaseNodeGrpcServer->>CryptoFactories: generate_coinbase_with_wallet_output(...)
CryptoFactories-->>BaseNodeGrpcServer: Output, Kernel, SecretKey
BaseNodeGrpcServer-->>Node: Block template with coinbase
Suggested reviewers
Poem
✨ 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:
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: 5
🔭 Outside diff range comments (2)
applications/minotari_node/src/grpc/base_node_grpc_server.rs (2)
1008-1024:⚠️ Potential issueReject empty coin‑base list early to avoid invalid blocks.
coinbasescan legally be an empty vector – nothing prevents a client from sending an empty list.
If this happens, the subsequent loop is skipped and the code continues with:
last_kernelleft at its default value,total_excess/noncestill zero,- an all‑zero
kernel_message.The call to
KernelBuilder::new()later will happily build a kernel, but the resulting block is invalid andunwrap()will panic as soon as validation runs.Add an explicit guard:
let mut coinbases: Vec<tari_rpc::NewBlockCoinbase> = request.coinbases; +if coinbases.is_empty() { + return Err(obscure_error_if_true( + report_error_flag, + Status::invalid_argument("At least one coinbase must be supplied".to_string()), + )); +}
1123-1147:⚠️ Potential issueAvoid
unwrap()– propagate kernel‑build errors to the client.A malformed aggregated signature will cause
KernelBuilder::build()to return an error, yet the current code panics with.unwrap(), potentially crashing the gRPC worker thread.- let kernel_new = KernelBuilder::new() + let kernel_new = KernelBuilder::new() .with_fee(0.into()) .with_features(last_kernel.features) .with_lock_height(last_kernel.lock_height) .with_excess(&CompressedCommitment::from_commitment(total_excess)) - .with_signature(Signature::new_from_schnorr(kernel_signature)) - .build() - .unwrap(); + .with_signature(Signature::new_from_schnorr(kernel_signature)) + .build() + .map_err(|e| obscure_error_if_true(report_error_flag, Status::internal(e.to_string())))?;This keeps the server robust and returns a meaningful gRPC error to the caller.
🧹 Nitpick comments (9)
base_layer/core/src/transactions/transaction_key_manager/mod.rs (1)
39-39: New function exports align with the key manager removal objective.The re-exports of
get_metadata_signatureandget_partial_txo_kernel_signature_for_coinbaseprovide direct access to these cryptographic operations without requiring the full key manager implementation. This supports the PR's goal of removing key manager dependency from coinbase creation.This architectural change promotes better separation of concerns by decoupling coinbase creation from the key manager, which should improve performance for the coinbase generation path.
integration_tests/src/miner.rs (2)
255-266:create_block_template_with_coinbasestill accepts unused parameters
key_managerandscript_key_idare no longer referenced in the body, producing dead‑code warnings and confusing future readers.-async fn create_block_template_with_coinbase( - base_client: &mut BaseNodeClient, - weight: u64, - key_manager: &MemoryDbKeyManager, - script_key_id: &TariKeyId, - ... -) -> (NewBlockTemplate, RistrettoSecretKey) { +async fn create_block_template_with_coinbase( + base_client: &mut BaseNodeClient, + weight: u64, + ... +) -> (NewBlockTemplate, RistrettoSecretKey) {If you really need the old signature for API stability, prefix them with an underscore to silence warnings and add a TODO explaining future removal.
285-307: Out‑of‑date variable namecoinbase_wallet_outputmisleads – it’s now a secret keySince
generate_coinbase_with_wallet_outputreturns aRistrettoSecretKeyas the 4‑th tuple element, keeping the old name implies the object is aWalletOutput. Rename it for clarity:- let (_, coinbase_output, coinbase_kernel, coinbase_wallet_output) = generate_coinbase_with_wallet_output( + let (_, coinbase_output, coinbase_kernel, coinbase_secret_key) = generate_coinbase_with_wallet_output(Also update the returned tuple at the end of the function.
base_layer/core/src/test_helpers/mod.rs (2)
75-85:default_coinbase_entitiesno longer needskey_manager– remove unused dependency
key_manageris now only used for an import that is ignored. Storing keys that are never referenced adds IO cost and makes the helper misleading.-pub async fn default_coinbase_entities(key_manager: &MemoryDbKeyManager) -> TariAddress { +pub fn default_coinbase_address() -> TariAddress {(Adjust call‑sites accordingly.)
95-98: Parameterkmis unused increate_blockThe function receives a
&MemoryDbKeyManagerbut never references it after the refactor. Either:
- use it (e.g. to persist
coinbase_secretfor later spends), or- drop it from the signature to prevent “unused variable” warnings.
base_layer/core/src/test_helpers/blockchain.rs (1)
668-685: Rename misleading local variableoutputs→coinbase_key(or similar).The second value returned by
create_blockis now aRistrettoSecretKey, i.e. one coin‑base spending key, not a collection of outputs.
Keeping the old name is confusing for future readers and may lead to erroneous assumptions in test‐helpers that use this tuple.- let (mut block, outputs) = create_block( + let (mut block, coinbase_key) = create_block( ... - (block, outputs) + (block, coinbase_key)applications/minotari_node/src/grpc/base_node_grpc_server.rs (1)
1125-1138: Zeroize secret material after use.
private_keysholds(spending_key, nonce_sk)tuples that remain in memory until the function returns.
Consider overwriting theseRistrettoSecretKeys after the signature aggregation loop (e.g., using thezeroizecrate) to reduce the window for in‑memory key leakage.base_layer/core/src/transactions/transaction_key_manager/inner.rs (1)
1713-1716: Typo:empheral_private_key→ephemeral_private_keyMaintaining consistent naming reduces cognitive load and prevents accidental mis‑references later.
- let (empheral_private_key, ephemeral_pubkey) = RistrettoPublicKey::random_keypair(&mut OsRng); + let (ephemeral_private_key, ephemeral_pubkey) = RistrettoPublicKey::random_keypair(&mut OsRng);base_layer/core/src/transactions/coinbase_builder.rs (1)
294-305: Excessive duplicate timing logsFive consecutive
debug!(... "build coinbase 1")lines capture virtually the same elapsed time, cluttering logs.Consider:
debug!(target: LOG_TARGET, "Coinbase build – after key‑gen: {:?}", timer.elapsed());and log once per meaningful milestone.
📜 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 (15)
applications/minotari_app_grpc/src/authentication/server_interceptor.rs(2 hunks)applications/minotari_merge_mining_proxy/src/block_template_manager.rs(4 hunks)applications/minotari_miner/src/run_miner.rs(3 hunks)applications/minotari_node/Cargo.toml(2 hunks)applications/minotari_node/src/grpc/base_node_grpc_server.rs(17 hunks)base_layer/core/src/chain_storage/blockchain_database.rs(11 hunks)base_layer/core/src/test_helpers/blockchain.rs(10 hunks)base_layer/core/src/test_helpers/mod.rs(5 hunks)base_layer/core/src/transactions/coinbase_builder.rs(17 hunks)base_layer/core/src/transactions/transaction_components/wallet_output.rs(4 hunks)base_layer/core/src/transactions/transaction_key_manager/inner.rs(18 hunks)base_layer/core/src/transactions/transaction_key_manager/mod.rs(2 hunks)base_layer/tari_mining_helper_ffi/src/error.rs(0 hunks)base_layer/tari_mining_helper_ffi/src/lib.rs(2 hunks)integration_tests/src/miner.rs(5 hunks)
💤 Files with no reviewable changes (1)
- base_layer/tari_mining_helper_ffi/src/error.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
base_layer/core/src/transactions/transaction_key_manager/mod.rs (1)
base_layer/core/src/transactions/transaction_key_manager/inner.rs (3)
get_metadata_signature(1325-1367)get_metadata_signature(1701-1747)get_partial_txo_kernel_signature_for_coinbase(1861-1881)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Cucumber tests / FFI
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: cargo check with stable
🔇 Additional comments (20)
applications/minotari_node/Cargo.toml (2)
20-20: Added tari_crypto dependency.The addition of
tari_cryptoas a workspace dependency is appropriate for the refactoring of coinbase generation to use direct cryptographic operations instead of key manager abstractions.
62-62: Added rand dependency.Adding the
randcrate as a dependency is necessary for random keypair generation in the new direct cryptographic key handling approach for coinbase generation.applications/minotari_app_grpc/src/authentication/server_interceptor.rs (2)
23-24: Added timing import.Adding
std::time::Instantfor performance measurement.
83-93: Added timing instrumentation to authentication.The change adds performance monitoring to measure and log how long the authentication process takes. This is a good addition for performance profiling without altering the authentication logic itself.
applications/minotari_miner/src/run_miner.rs (2)
30-32: Reorganized imports.Minor import refactoring to consolidate import statements without changing functionality.
Also applies to: 36-38, 41-42, 52-53
418-430: Refactored coinbase generation to use CryptoFactories.This is a key change in the PR, replacing the key manager dependency with direct cryptographic operations using
CryptoFactories. The code initializes a default CryptoFactories instance and passes it togenerate_coinbaseinstead of the previouskey_managerargument.base_layer/tari_mining_helper_ffi/src/lib.rs (2)
46-47: Imported CryptoFactories instead of key manager.Replaced the import of
create_memory_db_key_managerwithCryptoFactoriesto align with the PR objective of removing keymanager dependencies.
382-397: Refactored coinbase generation to use CryptoFactories.This change removes the key manager dependency and instead uses a default
CryptoFactoriesinstance for coinbase generation. This simplifies the code by making it synchronous and eliminating the need for key manager initialization and error handling.base_layer/core/src/transactions/transaction_key_manager/mod.rs (1)
28-29: Import consolidation improves readability.The consolidated imports make the code more concise and easier to maintain while preserving the same functionality.
base_layer/core/src/transactions/transaction_components/wallet_output.rs (4)
46-47: Import consolidation improves readability.The consolidated import makes the code more concise while preserving the same functionality.
255-258: Formatting improvement for public key addition.Reformatting the addition operation to a single line maintains the same functionality while making the code more concise.
281-283: Formatting improvement for signature addition.Reformatting the signature addition operation to a single line maintains the same functionality while making the code more concise.
353-355: Formatting improvement for size calculation.Reformatting the summation of serialized sizes to a single line maintains the same functionality while making the code more concise.
applications/minotari_merge_mining_proxy/src/block_template_manager.rs (4)
33-42: Updated imports to support key manager removal refactoring.The reorganized imports and the addition of
CryptoFactoriesreflect the transition from key manager to direct cryptographic operations.
65-65: Added CryptoFactories field to support synchronous coinbase generation.The new
crypto_factoryfield will enable theBlockTemplateManagerto perform coinbase operations without requiring the key manager.
84-85: Initialized CryptoFactories with default settings.Proper initialization of the new
crypto_factoryfield with default settings.
245-258:✅ Verification successful
Updated coinbase generation to use CryptoFactories instead of key manager.
The
generate_coinbasecall now uses thecrypto_factoryfield instead of the key manager, aligning with the PR objective of making coinbase generation synchronous and independent of the key manager interface.
🏁 Script executed:
#!/bin/bash # Check if the generate_coinbase function properly accepts CryptoFactories # Look for the updated function signature in the codebase rg "fn generate_coinbase.*?CryptoFactories" --type rustLength of output: 57
I couldn’t locate the
generate_coinbasedefinition or signature change with the previous regex. Let’s list all occurrences to verify its signature:
🏁 Script executed:
#!/bin/bash # List all generate_coinbase references to find its definition/signature rg "generate_coinbase" -n --type rustLength of output: 1796
Let’s confirm that
generate_coinbasenow takes a&CryptoFactoriesas its first parameter:
🏁 Script executed:
#!/bin/bash # Display the generate_coinbase signature rg -n "pub async fn generate_coinbase" -n base_layer/core/src/transactions/coinbase_builder.rs -A5Length of output: 504
Coinbase generation now uses CryptoFactories
Confirmed that the
generate_coinbasefunction signature in
base_layer/core/src/transactions/coinbase_builder.rshas been updated to:pub async fn generate_coinbase( crypto_factories: &CryptoFactories, fee: MicroMinotari, reward: MicroMinotari, height: u64, extra: &CoinBaseExtra, … )and that the call in
applications/minotari_merge_mining_proxy/src/block_template_manager.rs(lines 245–258) correctly passes&self.crypto_factory.
No further changes required.base_layer/core/src/transactions/coinbase_builder.rs (3)
134-145: API/ABI breakage – removedscript_keywhile tests and downstream code still depend on it
CoinbaseBuilderno longer stores ascript_key, yet the test module (and likely external callers) still invoke
with_script_key_id/with_script_key. This will lead to compilation errors.Please either
- re‑introduce the field & builder method, or
- update all call‑sites (tests, miners, FFI, etc.) in the same PR.
254-260: Public signature ofbuild_with_rewardchangedThe function now returns
(Transaction, RistrettoSecretKey)instead of the previous(Transaction, WalletOutput)tuple.
This breaks the external contract — make sure every caller is migrated; otherwise the project will fail to compile.If the extra return value is internal‑only, consider a dedicated accessor instead of altering the builder’s public API.
313-339: Local helperencrypt_data_for_recoveryduplicates logic from key‑managerDuplicated crypto code risks divergence and subtle bugs. Prefer extracting this helper to a common
module or re‑usingTransactionKeyManagerInner::encrypt_data_for_recovery.[ suggest_optional_refactor ]
| // let tip_header = db.fetch_tip_header()?; | ||
| // if header.height != tip_header.height() + 1 { | ||
| // return Err(ChainStorageError::InvalidArguments { | ||
| // func: "prepare_new_block", | ||
| // arg: "template", | ||
| // message: format!( | ||
| // "Expected new block template height to be {} but was {}", | ||
| // tip_header.height() + 1, | ||
| // header.height | ||
| // ), | ||
| // }); | ||
| // } | ||
| // if header.prev_hash != *tip_header.hash() { | ||
| // return Err(ChainStorageError::InvalidArguments { | ||
| // func: "prepare_new_block", | ||
| // arg: "template", | ||
| // message: format!( | ||
| // "Expected new block template previous hash to be set to the current tip hash ({}) but was {}", | ||
| // tip_header.hash(), | ||
| // header.prev_hash, | ||
| // ), | ||
| // }); | ||
| // } | ||
|
|
||
| // let timestamps = fetch_headers(&*db, min_height, prev_block_height)? | ||
| // .iter() | ||
| // .map(|h| h.timestamp) | ||
| // .collect::<Vec<_>>(); | ||
| // if timestamps.is_empty() { | ||
| // return Err(ChainStorageError::DataInconsistencyDetected { | ||
| // function: "prepare_new_block", | ||
| // details: format!( | ||
| // "No timestamps were returned within heights {} - {} by the database despite the tip header height \ | ||
| // being {}", | ||
| // min_height, | ||
| // prev_block_height, | ||
| // tip_header.height() | ||
| // ), | ||
| // }); | ||
| // } | ||
|
|
||
| // let median_timestamp = calc_median_timestamp(×tamps)?; | ||
| // If someone advanced the median timestamp such that the local time is less than the median timestamp, we need | ||
| // to increase the timestamp to be greater than the median timestamp otherwise the block wont be accepted by | ||
| // nodes | ||
| if median_timestamp > header.timestamp { | ||
| header.timestamp = median_timestamp | ||
| .checked_add(EpochTime::from(1)) | ||
| .ok_or(ChainStorageError::UnexpectedResult("Timestamp overflowed".to_string()))?; | ||
| } | ||
| // if median_timestamp > header.timestamp { | ||
| // header.timestamp = median_timestamp | ||
| // .checked_add(EpochTime::from(1)) | ||
| // .ok_or(ChainStorageError::UnexpectedResult("Timestamp overflowed".to_string()))?; | ||
| // } | ||
| let mut block = Block { header, body }; |
There was a problem hiding this comment.
Silent removal of header‑level sanity checks introduces consensus‑breaking risk
All the checks for
- height continuity (
header.height == tip.height + 1), prev_hashlinkage,- median‑time‑past adjustment, and
- timestamp future‑limit validation
have been commented out.
Without them, an attacker (or faulty caller) can submit a template that
– skips heights,
– references an arbitrary predecessor, or
– carries a timestamp far in the past / future,
yet the node will still happily assemble the block and later try to import it, leading to a guaranteed reject by full‑nodes or—even worse—a divergence in chains if other defensive layers are also removed.
These validations are cheap and executed before any expensive hash/MR work.
Please re‑enable them or, if they must be bypassed for a specific test harness, guard them behind a compile‑time/test‑only feature flag (e.g. #[cfg(feature="insecure_template")]).
| // body.sort(); | ||
| let mut header = BlockHeader::from(header); | ||
| let prev_block_height = header.height - 1; | ||
| let min_height = header.height.saturating_sub( | ||
| self.consensus_manager | ||
| .consensus_constants(header.height) | ||
| .median_timestamp_count() as u64, | ||
| ); | ||
| // let prev_block_height = header.height - 1; | ||
| // let min_height = header.height.saturating_sub( | ||
| // self.consensus_manager | ||
| // .consensus_constants(header.height) |
There was a problem hiding this comment.
body.sort() must not be commented out – MMR roots rely on canonical ordering
calculate_mmr_roots() is later invoked with the assumption that block.body.is_sorted() is true.
With the body.sort() call disabled, a template that is already unsorted would happily proceed until calculate_mmr_roots() returns InvalidBlock at runtime.
Please restore the sort (or reject an unsorted template up‑front).
-// body.sort();
+ body.sort();Committable suggestion skipped: line range outside the PR's diff.
| // world.utxos.insert(coinbase_name, wallet_output); | ||
| todo!("No longer implemented ^"); | ||
| mine_block_without_wallet_with_template(&mut client, template).await; |
There was a problem hiding this comment.
todo!("No longer implemented ^"); will panic in runtime tests
If the world‑state UTXO tracking is indeed obsolete, delete the block; if it is still required, implement the new behaviour before merging. Leaving a todo!() makes the integration test always abort once this path is hit.
- // world.utxos.insert(coinbase_name, wallet_output);
- todo!("No longer implemented ^");
+ // TODO: either re‑introduce UTXO tracking or drop the parameter entirely📝 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.
| // world.utxos.insert(coinbase_name, wallet_output); | |
| todo!("No longer implemented ^"); | |
| mine_block_without_wallet_with_template(&mut client, template).await; | |
| // TODO: either re‑introduce UTXO tracking or drop the parameter entirely | |
| mine_block_without_wallet_with_template(&mut client, template).await; |
| if self.crypto_factories.range_proof.range() < 64 | ||
| && value >= 1u64.shl(&self.crypto_factories.range_proof.range()) | ||
| { |
There was a problem hiding this comment.
Compile‑time failure in range‑check due to incorrect shl usage
u64::shl expects a value, not a reference, and the RHS must be an integer ‑ not usize.
This line will not compile, breaking the whole crate.
- if self.crypto_factories.range_proof.range() < 64
- && value >= 1u64.shl(&self.crypto_factories.range_proof.range())
+ let bits = self.crypto_factories.range_proof.range();
+ if bits < 64 && value >= (1u64 << bits)📝 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 self.crypto_factories.range_proof.range() < 64 | |
| && value >= 1u64.shl(&self.crypto_factories.range_proof.range()) | |
| { | |
| let bits = self.crypto_factories.range_proof.range(); | |
| if bits < 64 && value >= (1u64 << bits) { |
| let range_proof = if output_features.range_proof_type == RangeProofType::BulletProofPlus { | ||
| todo!("Bulletproofs range proof not implemented yet") | ||
| // Some( | ||
| // key_manager | ||
| // .construct_range_proof(&spending_key_id, value.into(), minimum_value_promise.into()) | ||
| // .await?, | ||
| // ) | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
todo!() will panic for BulletProof+ outputs
Calling build_with_reward with RangeProofType::BulletProofPlus triggers a runtime panic.
Return a proper error or construct the proof instead.
- if output_features.range_proof_type == RangeProofType::BulletProofPlus {
- todo!("Bulletproofs range proof not implemented yet")
+ if output_features.range_proof_type == RangeProofType::BulletProofPlus {
+ return Err(CoinbaseBuildError::BuildError(
+ "BulletProofPlus range‑proof generation not implemented".into(),
+ ));📝 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.
| let range_proof = if output_features.range_proof_type == RangeProofType::BulletProofPlus { | |
| todo!("Bulletproofs range proof not implemented yet") | |
| // Some( | |
| // key_manager | |
| // .construct_range_proof(&spending_key_id, value.into(), minimum_value_promise.into()) | |
| // .await?, | |
| // ) | |
| } else { | |
| None | |
| }; | |
| let range_proof = if output_features.range_proof_type == RangeProofType::BulletProofPlus { | |
| return Err(CoinbaseBuildError::BuildError( | |
| "BulletProofPlus range‑proof generation not implemented".into(), | |
| )); | |
| // Some( | |
| // key_manager | |
| // .construct_range_proof(&spending_key_id, value.into(), minimum_value_promise.into()) | |
| // .await?, | |
| // ) | |
| } else { | |
| None | |
| }; |
Test Results (CI)0 tests 0 ✅ 0s ⏱️ Results for commit 7edc3eb. |
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores
Tests