chore: upgrade pgp to 0.18 #7569
Conversation
WalkthroughThe pgp dependency is upgraded from 0.14.2 to 0.18, and the codebase is updated to use the new pgp::composed module types for SignedPublicKey and DetachedSignature. Additionally, the default TransportType variant is changed from Tcp to Tor. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
base_layer/p2p/src/auto_update/signature.rs (1)
143-159: Update test code to use pgp 0.17 API.The test code still references the old pgp API which will cause compilation errors. Update the types to use the
pgp::composednamespace.Apply this diff to fix the tests:
#[test] fn it_verifies_signed_message() { - let (sig, _) = pgp::StandaloneSignature::from_string(VALID_SIGNATURE.trim()).unwrap(); - let (key, _) = pgp::SignedPublicKey::from_string(PUBLIC_KEY).unwrap(); + let (sig, _) = pgp::composed::DetachedSignature::from_string(VALID_SIGNATURE.trim()).unwrap(); + let (key, _) = pgp::composed::SignedPublicKey::from_string(PUBLIC_KEY).unwrap(); let verifier = SignedMessageVerifier::new(vec![key]); let signer = verifier.verify_signature(&sig, MESSAGE).unwrap(); - let (maintainer, _) = pgp::SignedPublicKey::from_string(PUBLIC_KEY).unwrap(); + let (maintainer, _) = pgp::composed::SignedPublicKey::from_string(PUBLIC_KEY).unwrap(); assert_eq!(*signer, maintainer); } #[test] fn it_does_not_validate_with_tampered_message() { - let (sig, _) = pgp::StandaloneSignature::from_string(VALID_SIGNATURE.trim()).unwrap(); + let (sig, _) = pgp::composed::DetachedSignature::from_string(VALID_SIGNATURE.trim()).unwrap(); let verifier = SignedMessageVerifier::new(maintainers().collect()); assert!(verifier.verify_signature(&sig, "Zilip R. Phimmermann").is_none()); }
📜 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 (3)
base_layer/p2p/Cargo.toml(1 hunks)base_layer/p2p/src/auto_update/mod.rs(3 hunks)base_layer/p2p/src/auto_update/signature.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
base_layer/p2p/src/auto_update/signature.rs (1)
base_layer/p2p/src/auto_update/mod.rs (1)
maintainers(205-210)
base_layer/p2p/src/auto_update/mod.rs (1)
base_layer/p2p/src/auto_update/signature.rs (1)
new(31-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: wasm build tests
- GitHub Check: ledger build tests
- GitHub Check: ci
- GitHub Check: cargo check with stable
- GitHub Check: cargo check with msrv
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: Cucumber tests / FFI
🔇 Additional comments (6)
base_layer/p2p/src/auto_update/mod.rs (3)
26-26: LGTM!The import correctly uses the new
pgp::composed::Deserializablepath for pgp 0.17.
190-196: LGTM!The function signature and deserialization logic have been correctly updated to use
pgp::composed::DetachedSignaturefrom pgp 0.17.
205-210: LGTM!The iterator return type and key deserialization have been correctly migrated to use
pgp::composed::SignedPublicKeyfrom pgp 0.17.base_layer/p2p/src/auto_update/signature.rs (3)
27-33: LGTM!The struct field and constructor have been correctly updated to use
pgp::composed::SignedPublicKeyfrom pgp 0.17.
35-52: LGTM!The method signature correctly uses
pgp::composed::DetachedSignaturefrom pgp 0.17.
54-62: LGTM!The helper method signature has been correctly updated to use
pgp::composed::DetachedSignatureandpgp::composed::SignedPublicKeyfrom pgp 0.17.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
base_layer/transaction_components/src/transaction_components/output_type.rs (1)
48-52: Add a quick test asserting the default variantTo lock this change in, assert that OutputType::default() == Standard.
Apply within the existing tests:
@@ #[test] fn it_converts_from_byte_to_output_type() { + assert_eq!(OutputType::default(), OutputType::Standard); assert_eq!(OutputType::from_byte(0), Some(OutputType::Standard));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
applications/minotari_console_wallet/src/automation/commands.rs(1 hunks)applications/minotari_console_wallet/src/ui/state/app_state.rs(1 hunks)applications/minotari_node/src/commands/parser.rs(1 hunks)base_layer/common_types/src/payment_reference.rs(1 hunks)base_layer/core/tests/tests/block_validation.rs(1 hunks)base_layer/p2p/src/auto_update/signature.rs(4 hunks)base_layer/p2p/src/transport.rs(2 hunks)base_layer/transaction_components/src/transaction_components/output_type.rs(1 hunks)base_layer/transaction_components/src/transaction_components/range_proof_type.rs(1 hunks)base_layer/transaction_components/src/transaction_components/test.rs(2 hunks)base_layer/wallet/src/transaction_service/config.rs(1 hunks)comms/dht/examples/memorynet.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-06-14T04:56:43.247Z
Learnt from: hansieodendaal
Repo: tari-project/tari PR: 7216
File: base_layer/core/src/base_node/comms_interface/comms_request.rs:146-148
Timestamp: 2025-06-14T04:56:43.247Z
Learning: In the Tari codebase, `HashOutput` is a type alias for `FixedHash`, and `FixedHash` already implements `Display` trait with hex formatting by calling `self.0.to_hex()` internally. This means when formatting `HashOutput` values, there's no need to explicitly call `.to_hex()` as the Display implementation handles hex conversion automatically.
Applied to files:
base_layer/transaction_components/src/transaction_components/output_type.rsapplications/minotari_node/src/commands/parser.rs
📚 Learning: 2025-10-03T10:21:49.027Z
Learnt from: hansieodendaal
Repo: tari-project/tari PR: 7514
File: base_layer/transaction_components/src/transaction_builder/builder.rs:704-752
Timestamp: 2025-10-03T10:21:49.027Z
Learning: In base_layer/transaction_components/src/transaction_builder/builder.rs, the change_encrypted_data_if_fee_changed helper should only update fees in MemoField instances that already contain a fee (when get_fee() returns Some). It should not set fees in memos where get_fee() returns None, as not all outputs require fees in their memos.
Applied to files:
base_layer/transaction_components/src/transaction_components/test.rsapplications/minotari_console_wallet/src/ui/state/app_state.rs
📚 Learning: 2025-05-23T07:49:57.349Z
Learnt from: hansieodendaal
Repo: tari-project/tari PR: 6963
File: common_sqlite/src/error.rs:88-92
Timestamp: 2025-05-23T07:49:57.349Z
Learning: In the StorageError enum in common_sqlite/src/error.rs, the HexError variant should keep the manual From<HexError> implementation rather than using #[from] attribute, as it stores a String representation of the error rather than the HexError type itself.
Applied to files:
applications/minotari_node/src/commands/parser.rs
📚 Learning: 2025-07-09T08:33:29.320Z
Learnt from: hansieodendaal
Repo: tari-project/tari PR: 7294
File: comms/dht/src/network_discovery/seed_strap.rs:352-456
Timestamp: 2025-07-09T08:33:29.320Z
Learning: In comms/dht/src/network_discovery/seed_strap.rs, the NUM_RETRIES logic in get_peers is specifically designed to handle peer connections that are closed while trying to RPC stream peer info, not general connection failures. The retry logic only applies when peers.is_empty() && !conn.is_connected() && attempt < NUM_RETRIES, which indicates a mid-stream disconnection.
Applied to files:
comms/dht/examples/memorynet.rs
📚 Learning: 2025-07-09T08:33:29.320Z
Learnt from: hansieodendaal
Repo: tari-project/tari PR: 7294
File: comms/dht/src/network_discovery/seed_strap.rs:352-456
Timestamp: 2025-07-09T08:33:29.320Z
Learning: In comms/dht/src/network_discovery/seed_strap.rs, the fetch_peers_from_connection and collect_peer_stream functions rely on RPC streaming, and when the main connection is closed by another process, collect_peer_stream times out after STREAM_ITEM_TIMEOUT because it cannot detect that the peer can no longer respond, returning an empty vector of peers. This is why the connection state check is important for the retry logic.
Applied to files:
comms/dht/examples/memorynet.rs
📚 Learning: 2025-06-16T14:49:31.831Z
Learnt from: martinserts
Repo: tari-project/tari PR: 7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
Applied to files:
applications/minotari_console_wallet/src/automation/commands.rsbase_layer/core/tests/tests/block_validation.rs
📚 Learning: 2025-07-15T12:23:14.650Z
Learnt from: hansieodendaal
Repo: tari-project/tari PR: 7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.650Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
Applied to files:
applications/minotari_console_wallet/src/automation/commands.rsapplications/minotari_console_wallet/src/ui/state/app_state.rs
📚 Learning: 2025-08-11T15:37:17.354Z
Learnt from: hansieodendaal
Repo: tari-project/tari PR: 7387
File: base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs:2896-2927
Timestamp: 2025-08-11T15:37:17.354Z
Learning: The `update_accumulated_difficulty` method in `base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs` is migration-specific code that only runs during a once-off migration when existing ChainHeader data is present in LMDB. The consistency guarantees are upheld by the calling function `process_accumulated_data_for_height`, so additional validation within this method is redundant.
Applied to files:
applications/minotari_console_wallet/src/automation/commands.rs
📚 Learning: 2025-09-08T08:43:50.910Z
Learnt from: MCozhusheck
Repo: tari-project/tari PR: 7478
File: base_layer/p2p/src/signature_verification/gpg_keys/seed_peers_http.asc:1-15
Timestamp: 2025-09-08T08:43:50.910Z
Learning: In the Tari codebase, temporary test PGP keys for signature verification are acceptable during development phases when they will be replaced with production keys before final release. The maintainers prefer to keep the implementation simple during drafts rather than over-engineering with test-only conditionals for temporary keys.
Applied to files:
base_layer/p2p/src/auto_update/signature.rs
🧬 Code graph analysis (3)
base_layer/transaction_components/src/transaction_components/test.rs (1)
base_layer/transaction_components/src/transaction_components/transaction_output.rs (2)
outputs(545-549)outputs(572-576)
comms/dht/examples/memorynet.rs (1)
comms/dht/examples/memory_net/utilities.rs (1)
make_node(639-659)
base_layer/p2p/src/auto_update/signature.rs (1)
base_layer/p2p/src/auto_update/mod.rs (1)
maintainers(205-210)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: cargo check with msrv
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: cargo check with stable
- GitHub Check: wasm build tests
- GitHub Check: ledger build tests
- GitHub Check: ci
- GitHub Check: Cucumber tests / FFI
- GitHub Check: Cucumber tests / Base Layer
🔇 Additional comments (16)
applications/minotari_console_wallet/src/ui/state/app_state.rs (1)
753-754: LGTM: Efficient migration from Vec to slice reference.The change from
vec![txn.into()]to&[txn.into()]is a good optimization that avoids unnecessary heap allocation for a single-item collection. The temporary array lives long enough for the function call and await, and this aligns with the function signature expecting&[CompletedTransaction].comms/dht/examples/memorynet.rs (1)
86-95: LGTM! Appropriate use of array literal for fixed-size collection.The change from a Vec to an array for the single seed node is correct and slightly more efficient (stack-allocated vs heap-allocated). Since
seed_nodeis never mutated and always contains exactly one element, an array is the appropriate choice.Note: This refactor appears unrelated to the pgp upgrade mentioned in the PR title, but it's a reasonable cleanup.
base_layer/transaction_components/src/transaction_components/test.rs (2)
252-260: LGTM! Array literal is a valid optimization.The change from
vec![...]to an array literal[...]is functionally equivalent and slightly more efficient since it avoids an initial heap allocation before collecting references.
263-266: LGTM! Consistent with the earlier change.The tampering test correctly uses the same array literal pattern, maintaining consistency with the earlier modification while preserving the test's validation logic.
base_layer/p2p/src/transport.rs (1)
89-103: Default TransportType is now Tor — confirm intended behaviorDeriving Default with #[default] on Tor makes TransportConfig::default() choose Tor by default. If this differs from the prior impl, it alters out-of-the-box transport. Please confirm this default is intentional and update docs/config examples if needed.
base_layer/transaction_components/src/transaction_components/range_proof_type.rs (1)
43-47: LGTM: derive(Default) with BulletProofPlus as defaultMatches existing tests and reduces boilerplate.
applications/minotari_node/src/commands/parser.rs (1)
39-45: LGTM: derive(Default) for Format with Text defaultRemoves boilerplate; behavior remains Text by default.
base_layer/wallet/src/transaction_service/config.rs (1)
94-100: LGTM; confirm default routing remains unchangedDefault is now provided via derive(Default) with DirectAndStoreAndForward. Please confirm this matches the prior manual Default and update any docs if they mention defaults explicitly.
base_layer/common_types/src/payment_reference.rs (1)
178-187: LGTM: default PayRef display is ShortenedGood ergonomic default; implementation remains straightforward.
base_layer/core/tests/tests/block_validation.rs (1)
294-296: LGTM: migrated to slice-based get_script_offsetCallsite matches new signature and retains intent.
base_layer/p2p/src/auto_update/signature.rs (6)
26-33: Type migration looks correct and consistent.The struct and constructor have been properly updated to use
pgp::composed::SignedPublicKey. The relatedmaintainers()function inauto_update/mod.rsalso returns this type, ensuring API compatibility.
35-52: Type migration to DetachedSignature is semantically appropriate.The change from
StandaloneSignaturetoDetachedSignaturealigns with the signature verification pattern where the signature is separate from the signed data (the hashes string).
67-67: Import update is consistent with the pgp 0.17 module structure.
144-152: Test updated correctly for new pgp types.The test properly uses
pgp::composed::DetachedSignatureandpgp::composed::SignedPublicKeywith thefrom_stringconstructor. The test logic remains unchanged.
154-159: Negative test case correctly updated.The test properly validates that signature verification fails with a tampered message using the new pgp types.
54-62: API compatibility verified for pgp 0.17.The
verify()method in pgp 0.17 accepts(&self, pub_key: &SignedPublicKey, data: &[u8])and returnsResult<()>. The code at line 61 correctly callssignature.verify(pk, message.as_bytes()).is_ok(), which matches this signature. Tests at lines 143–159 validate this functionality.
applications/minotari_console_wallet/src/automation/commands.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Looks good, except for the one issue that looks like a copy-paste problem.
| std::slice::from_ref(&party_info.pre_mine_script_key_id), | ||
| std::slice::from_ref(&party_info.pre_mine_script_key_id), |
There was a problem hiding this comment.
This seems wrong - copy-paste problem?
8fe8028 to
1e9684e
Compare
Description
upgrades pgp to a newer version
Summary by CodeRabbit
Dependencies
Bug Fixes/Improvements
✏️ Tip: You can customize this high-level summary in your review settings.