Lots of fixes and updated to the latest node version#624
Lots of fixes and updated to the latest node version#624ginnun wants to merge 13 commits intotxpipe:mainfrom
Conversation
|
|
||
| pub type RewardAccount = Bytes; | ||
| #[derive(Debug, Clone, Eq, PartialEq, PartialOrd, Ord)] | ||
| pub struct FieldedRewardAccount{ |
There was a problem hiding this comment.
is the FieldedRewardAccount struct defined as part of some spec or is it a quality-of-life improvement for dealing with credentials?
If its the latter, I would prefer to have this in a higher layer of the stack and not as part of the codec of the miniprotocol.
There was a problem hiding this comment.
@scarmuega It's there to be able to produce the same error message with Haskell codebase. Since Rust doesn't have type specialization, old-good RewardAccount (which is a Byte) can't be used for HaskellDisplay to distinguish.
So not sure if this is a quality-of-life improvement or not. If do you think so, I could refactor it. Just tell me what layer it should be in that case. Thanks.
ginnun
changed the title
|
Please note that failing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds FieldedRewardAccount (CBOR Encode/Decode), Plutus v4 cost-model support, new mismatch container MismatchArr, public DecoderError/DeserialiseFailure, manual minicbor Decode for Tx<'b>, widespread replacement of DisplayRewardAccount with FieldedRewardAccount, and numerous ordering/typing adjustments across network, protocol, display, and error layers. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CBOR as "CBOR Decoder"
participant TxDec as "Tx<'b>::Decode"
participant Body as "TransactionBody"
participant Wits as "WitnessSet"
CBOR->>TxDec: start decode()
TxDec->>CBOR: read array header
TxDec->>Body: decode(transaction_body) with KeepRaw context
TxDec->>Wits: decode(witness_set) with KeepRaw context
alt success field present
TxDec->>CBOR: decode bool success
else
Note right of TxDec: success defaults to true
end
alt auxiliary_data present
TxDec->>CBOR: decode Nullable<AuxiliaryData>
else
Note right of TxDec: auxiliary_data defaults to Null
end
TxDec-->>CBOR: return Tx
sequenceDiagram
autonumber
participant Enc as "Encoder"
participant FE as "FieldedRewardAccount"
participant Buf as "Raw CBOR Bytes"
Enc->>FE: request serialization (Encode)
FE-->>Enc: provide 29-byte payload (1-byte header + 28-byte stake hash)
Enc->>Buf: write raw bytes
Buf->>FE: raw bytes read (Decode)
FE->>FE: TryFrom<&[u8]> parsing and validate 29 bytes
FE-->>Buf: FieldedRewardAccount instance
sequenceDiagram
autonumber
participant Codec as "localtxsubmission::codec"
participant Stream as "Input Stream"
participant Err as "DecoderError"
Codec->>Stream: attempt to decode error payload
alt decodes known error tag (incl. VRFKeyHashAlreadyRegistered)
Stream-->>Codec: Ok(error struct)
Codec-->>Codec: return Some(error)
else
Stream-->>Codec: Err(e)
Codec->>Err: propagate/convert to DecoderError variants
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
pallas-codec/src/utils.rs(1 hunks)pallas-hardano/src/display/haskell_display.rs(29 hunks)pallas-hardano/src/display/haskell_error.rs(5 hunks)pallas-network/src/miniprotocols/localstate/queries_v16/codec.rs(1 hunks)pallas-network/src/miniprotocols/localstate/queries_v16/mod.rs(14 hunks)pallas-network/src/miniprotocols/localtxsubmission/codec.rs(0 hunks)pallas-network/src/miniprotocols/localtxsubmission/primitives.rs(3 hunks)pallas-network/src/miniprotocols/localtxsubmission/protocol.rs(10 hunks)pallas-primitives/src/conway/model.rs(2 hunks)
💤 Files with no reviewable changes (1)
- pallas-network/src/miniprotocols/localtxsubmission/codec.rs
🧰 Additional context used
🧬 Code graph analysis (3)
pallas-network/src/miniprotocols/localtxsubmission/protocol.rs (1)
pallas-network/src/miniprotocols/localstate/queries_v16/mod.rs (8)
from(635-654)from(844-846)from(908-910)from(914-916)from(920-922)from(929-931)from(935-937)from(958-960)
pallas-primitives/src/conway/model.rs (1)
pallas-codec/src/utils.rs (18)
decode(22-33)decode(148-161)decode(326-345)decode(420-430)decode(495-501)decode(537-543)decode(593-597)decode(631-637)decode(672-685)decode(758-769)decode(849-866)decode(896-917)decode(1065-1073)decode(1162-1172)decode(1273-1285)decode(1340-1355)d(151-151)d(329-329)
pallas-hardano/src/display/haskell_display.rs (3)
pallas-hardano/src/display/haskells_show_string.rs (1)
haskell_show_string(4-98)pallas-network/src/miniprotocols/localtxsubmission/codec.rs (16)
encode(39-54)encode(62-87)encode(130-140)encode(154-169)encode(184-194)encode(232-253)encode(320-328)encode(344-353)encode(384-402)encode(413-420)encode(435-446)encode(468-509)encode(632-766)encode(784-795)encode(842-872)encode(899-919)pallas-traverse/src/governance.rs (1)
reward_account(24-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test Suite (windows-latest, stable)
- GitHub Check: Test Suite (macOS-latest, stable)
- GitHub Check: Check (windows-latest, stable)
- GitHub Check: Test Suite (ubuntu-latest, stable)
🔇 Additional comments (1)
pallas-codec/src/utils.rs (1)
1342-1348: LGTM – approve compatibility fix, verify Nullable decoding
The change fromd.null()/d.undefined()tod.skip()is safe given the priord.datatype()?check. Please run your pallas-codec tests locally (e.g.cargo test -p pallas-codec) to confirm no regressions in Nullable decoding.
pallas-network/src/miniprotocols/localstate/queries_v16/codec.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
pallas-network/src/miniprotocols/localtxsubmission/codec.rs (1)
335-347: Consider validating the CBOR array length for robustness.The
Decodeimplementation callsd.array()?but discards the returned length. SinceMismatchArris explicitly documented as a 2-element array, validating the length would catch malformed input early (e.g., extra elements would be silently ignored with the current implementation).That said, this pattern of not checking length is common in this codebase (e.g.,
VotingProcedure::decode), so this is a minor suggestion.🔧 Optional: Add length validation
impl<'b, T, C> Decode<'b, C> for super::MismatchArr<T> where T: Decode<'b, C>, { fn decode(d: &mut Decoder<'b>, ctx: &mut C) -> Result<Self, decode::Error> { - d.array()?; + let len = d.array()?; + if len != Some(2) { + return Err(decode::Error::message("Expected array of length 2 for MismatchArr")); + } let t0 = d.decode_with(ctx)?; let t1 = d.decode_with(ctx)?; Ok(super::MismatchArr(t0, t1)) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pallas-network/src/miniprotocols/localtxsubmission/codec.rs` around lines 335 - 347, The Decode impl for super::MismatchArr currently calls d.array()? but ignores the returned length; update it to capture the length (let len = d.array()?) and validate that it is exactly 2 when fixed-length (or allow None for indefinite-length CBOR), returning a decode error for any Some(n) where n != 2; then proceed to call d.decode_with(ctx)? twice and return super::MismatchArr(t0, t1). Ensure you reference the same symbols: Decode for super::MismatchArr, d.array(), d.decode_with(ctx) and super::MismatchArr(t0, t1) when implementing the check.pallas-network/src/miniprotocols/localstate/queries_v16/mod.rs (1)
460-524: Consider reducing duplication betweenProtocolParamandCurrentProtocolParam
CurrentProtocolParamappears to have the same fields asProtocolParam. If they are semantically identical, consider using a type alias or extracting a shared base to reduce maintenance burden. If there are subtle differences (e.g., different CBOR encoding requirements as hinted by the TODO on line 392), please add a comment explaining why both are needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pallas-network/src/miniprotocols/localstate/queries_v16/mod.rs` around lines 460 - 524, CurrentProtocolParam duplicates ProtocolParam field-for-field; either unify them by replacing CurrentProtocolParam with a type alias to ProtocolParam or extract a shared struct used by both (e.g., rename ProtocolParam -> ProtocolParamBase and make ProtocolParam and CurrentProtocolParam newtype wrappers), and update usages to the chosen shared type; if there are encoding differences (see the TODO about CBOR encoding near the ProtocolParam implementation), add a clear comment on why two distinct types must remain and document the specific encoding/semantic difference on ProtocolParam and CurrentProtocolParam so future maintainers understand the rationale.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pallas-hardano/src/display/haskell_display.rs`:
- Around line 526-535: The fallback use of Debug in DecoderError::to_haskell_str
breaks the Haskell wire format; replace the catch-all "{:?}" branch with
explicit, stable Haskell-style string formatting for every DecoderError variant.
Update the impl HaskellDisplay for DecoderError::to_haskell_str to match each
variant (not just DecoderError::DeserialiseFailure) and return a deterministic
string (e.g., "DecoderErrorVariantName <fields...>") using existing helper
methods like to_haskell_str()/to_haskell_str_p() where appropriate so all
variants produce stable, predictable output.
- Line 397: The TooManyCollateralInputs message is passing the arguments to
as_mismatch in the wrong order (as_mismatch expects (expected, supplied));
change the call in the TooManyCollateralInputs formatter from
as_mismatch(actual, max) to as_mismatch(max, actual) and make the same swap for
the other occurrences referenced (the similar block around the 664-672 range) so
the serialized mismatch uses expected=max and supplied=actual.
In `@pallas-hardano/src/display/haskell_error.rs`:
- Around line 22-29: The helper as_cbor_decode_failure currently panics by
calling unwrap() on serde_json::to_string; change its signature from fn
as_cbor_decode_failure(message: String, position: u64) -> String to return
Result<String, serde_json::Error>, remove the unwrap and return the Result from
serde_json::to_string(&error), and update any callers of as_cbor_decode_failure
to handle the Result (propagate or map_err as appropriate). Keep the
construction of DecoderError::DeserialiseFailure, TxCmdError::TxReadError and
TxSubmitFail::TxSubmitFail unchanged — only alter the return type and remove the
unwrap so serialization errors are propagated.
---
Nitpick comments:
In `@pallas-network/src/miniprotocols/localstate/queries_v16/mod.rs`:
- Around line 460-524: CurrentProtocolParam duplicates ProtocolParam
field-for-field; either unify them by replacing CurrentProtocolParam with a type
alias to ProtocolParam or extract a shared struct used by both (e.g., rename
ProtocolParam -> ProtocolParamBase and make ProtocolParam and
CurrentProtocolParam newtype wrappers), and update usages to the chosen shared
type; if there are encoding differences (see the TODO about CBOR encoding near
the ProtocolParam implementation), add a clear comment on why two distinct types
must remain and document the specific encoding/semantic difference on
ProtocolParam and CurrentProtocolParam so future maintainers understand the
rationale.
In `@pallas-network/src/miniprotocols/localtxsubmission/codec.rs`:
- Around line 335-347: The Decode impl for super::MismatchArr currently calls
d.array()? but ignores the returned length; update it to capture the length (let
len = d.array()?) and validate that it is exactly 2 when fixed-length (or allow
None for indefinite-length CBOR), returning a decode error for any Some(n) where
n != 2; then proceed to call d.decode_with(ctx)? twice and return
super::MismatchArr(t0, t1). Ensure you reference the same symbols: Decode for
super::MismatchArr, d.array(), d.decode_with(ctx) and super::MismatchArr(t0, t1)
when implementing the check.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
pallas-codec/src/utils.rspallas-hardano/src/display/haskell_display.rspallas-hardano/src/display/haskell_error.rspallas-network/src/miniprotocols/localstate/queries_v16/codec.rspallas-network/src/miniprotocols/localstate/queries_v16/mod.rspallas-network/src/miniprotocols/localtxsubmission/codec.rspallas-network/src/miniprotocols/localtxsubmission/primitives.rspallas-network/src/miniprotocols/localtxsubmission/protocol.rspallas-primitives/src/conway/model.rspallas-validate/tests/conway.rs
| #[derive(Debug, Serialize)] | ||
| #[serde(tag = "tag", content = "contents")] | ||
| pub enum TxCmdError { | ||
| SocketEnvError(String), |
There was a problem hiding this comment.
It seems currently unused, but I wonder, shouldn’t this be also renamed to TxCmdSocketEnvError? See here:
| #[derive(Debug, Serialize)] | ||
| pub enum DecoderError { |
There was a problem hiding this comment.
Hmmm, you're deriving Serialize, so by default this will result in "tagged" JSON (e.g {"DeserialiseFailure":[...]}).
But cardano-node is using toJSON = Aeson.String . textShow, so this means the will just have a string "DecoderErrorDeserialiseFailure \"Shelley Tx\" ..."? See:
Is this unused currently? Why don’t we test this path with testgen-hs? Or am I wrong?
There was a problem hiding this comment.
I did the intent clearer in ed422bc.
In short I replaced (serialize_with = "use_haskell_display") with a custom Serializer.
To your comment:
The #[derive(Serialize)] could have been safely removed, since it was never used on DecoderError. When TxCmdError::TxReadError(Vec<DecoderError>) is serialized, the #[serde(serialize_with = "use_haskell_display")] custom serialize was kicking in (line 134), which calls to_haskell_str() on each element and writes it as a plain string — matching Haskell's textShow behavior.
The test at line 156 confirms this:
"contents":["DecoderErrorDeserialiseFailure \"Shelley Tx\" (DeserialiseFailure 0 (\"expected list len or indef\"))"]
It's a string, not tagged JSON. So there is no problem in the implementation.
| e: &mut minicbor::Encoder<W>, | ||
| _ctx: &mut C, | ||
| ) -> Result<(), minicbor::encode::Error<W::Error>> { | ||
| let mut prefix: u8 = self.network.clone().into(); |
There was a problem hiding this comment.
Shouldn’t we have:
prefix |= 0b1110_0000;before the ScriptHash check? See here:
pallas/pallas-addresses/src/lib.rs
Lines 565 to 572 in 529f5d6
And then here:
pallas/pallas-addresses/src/lib.rs
Lines 557 to 563 in 529f5d6
There was a problem hiding this comment.
per CIP-19 we should. This was not effecting our use-case, thus slipped. Fixed in a03ae02.
| pub struct OHashMap<K, V>(pub Vec<(K, V)>); | ||
|
|
||
| #[derive(Encode, Decode, Debug, Clone, Eq, PartialEq)] | ||
| #[derive(Encode, Decode, Debug, Clone, Eq, PartialEq, PartialOrd, Ord)] |
There was a problem hiding this comment.
Hmmm, as a fieldless enum, I think this should derive Copy for ergonomics (no need to call .clone()). It will end up as a single byte after compilation anyway.
There was a problem hiding this comment.
Practially no difference, only cosmetics. I included this suggestion in a bigger commit: a03ae02
| #[n(0)] | ||
| StakePoolNotRegisteredOnKeyPOOL(#[n(0)] KeyHash), | ||
| #[n(1)] | ||
| StakePoolRetirementWrongEpochPOOL(#[n(0)] Mismatch<EpochNo>, #[n(1)] Mismatch<EpochNo>), |
There was a problem hiding this comment.
So the derived Encode will write 4 EpochNo fields to CBOR, right? But the wire protocol has only 3, according to the custom decode:
pallas/pallas-network/src/miniprotocols/localtxsubmission/codec.rs
Lines 262 to 273 in 529f5d6
Are we fine with this?
Perhaps we should automatically add one more test to each existing test, like cbor_data == encode(decode(cbor_data))?
To make sure our codecs are completely invertible.
There was a problem hiding this comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pallas-network/src/miniprotocols/localstate/queries_v16/mod.rs`:
- Around line 639-679: The current TryFrom<&[u8]> impl for FieldedRewardAccount
only validates the upper nibble (header_type) but reads the network via bytes[0]
& 0b0000_0001, which accepts non-canonical values like 0xe2/0xf3; update the
header/network validation in TryFrom to also verify the full lower nibble
(bytes[0] & 0x0F) is exactly the canonical values (0x0 for Testnet or 0x1 for
Mainnet) and return InvalidRewardAccount::InvalidHeaderType (or add a new
InvalidNetwork variant) when it is not; locate the check around header_type and
bytes[0] in the TryFrom impl and replace the single-bit test with a strict
lower-nibble match before mapping to Network.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dffa7f09-ba08-4f2e-be56-5f72beb877c8
📒 Files selected for processing (3)
pallas-network/src/miniprotocols/localstate/queries_v16/codec.rspallas-network/src/miniprotocols/localstate/queries_v16/mod.rspallas-network/src/miniprotocols/localtxsubmission/protocol.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- pallas-network/src/miniprotocols/localstate/queries_v16/codec.rs
| pub enum InvalidRewardAccount { | ||
| InvalidLength(usize), | ||
| InvalidHeaderType(u8), | ||
| } | ||
|
|
||
| impl fmt::Display for InvalidRewardAccount { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| match self { | ||
| Self::InvalidLength(len) => { | ||
| write!(f, "invalid reward account length: expected 29, got {len}") | ||
| } | ||
| Self::InvalidHeaderType(header) => { | ||
| write!(f, "invalid reward account header type: 0x{header:02x}") | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl std::error::Error for InvalidRewardAccount {} | ||
|
|
||
| impl TryFrom<&[u8]> for FieldedRewardAccount { | ||
| type Error = InvalidRewardAccount; | ||
|
|
||
| fn try_from(bytes: &[u8]) -> Result<Self, Self::Error> { | ||
| if bytes.len() != 29 { | ||
| return Err(InvalidRewardAccount::InvalidLength(bytes.len())); | ||
| } | ||
|
|
||
| // Header byte layout: [type(4 bits) | network(4 bits)] | ||
| // Upper nibble must be 0b1110 (key hash) or 0b1111 (script hash) | ||
| // per CIP-19 reward account address format. | ||
| let header_type = bytes[0] >> 4; | ||
| if !matches!(header_type, 0b1110 | 0b1111) { | ||
| return Err(InvalidRewardAccount::InvalidHeaderType(bytes[0])); | ||
| } | ||
|
|
||
| let network = if bytes[0] & 0b0000_0001 != 0 { | ||
| Network::Mainnet | ||
| } else { | ||
| Network::Testnet | ||
| }; |
There was a problem hiding this comment.
Reject non-canonical network nibbles in reward-account headers.
This still validates only the upper nibble. The bytes[0] & 0b0000_0001 check accepts headers like 0xe2 / 0xf3 and silently maps them to Testnet / Mainnet, so malformed reward accounts are still treated as valid and can be misreported downstream.
🔧 Proposed fix
pub enum InvalidRewardAccount {
InvalidLength(usize),
InvalidHeaderType(u8),
+ InvalidNetwork(u8),
}
impl fmt::Display for InvalidRewardAccount {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
Self::InvalidLength(len) => {
write!(f, "invalid reward account length: expected 29, got {len}")
}
Self::InvalidHeaderType(header) => {
write!(f, "invalid reward account header type: 0x{header:02x}")
}
+ Self::InvalidNetwork(network) => {
+ write!(f, "invalid reward account network id: 0x{network:01x}")
+ }
}
}
}
@@
- let network = if bytes[0] & 0b0000_0001 != 0 {
- Network::Mainnet
- } else {
- Network::Testnet
- };
+ let network = match bytes[0] & 0b0000_1111 {
+ 0 => Network::Testnet,
+ 1 => Network::Mainnet,
+ other => return Err(InvalidRewardAccount::InvalidNetwork(other)),
+ };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pallas-network/src/miniprotocols/localstate/queries_v16/mod.rs` around lines
639 - 679, The current TryFrom<&[u8]> impl for FieldedRewardAccount only
validates the upper nibble (header_type) but reads the network via bytes[0] &
0b0000_0001, which accepts non-canonical values like 0xe2/0xf3; update the
header/network validation in TryFrom to also verify the full lower nibble
(bytes[0] & 0x0F) is exactly the canonical values (0x0 for Testnet or 0x1 for
Mainnet) and return InvalidRewardAccount::InvalidHeaderType (or add a new
InvalidNetwork variant) when it is not; locate the check around header_type and
bytes[0] in the TryFrom impl and replace the single-bit test with a strict
lower-nibble match before mapping to Network.
3 participants
This PR contains lots of fixes, improvements and fixes some breaking changes that comes with the latest Cardano node version (for Haskell text generation)
Summary by CodeRabbit
New Features
Changes
Tests