feat: Support proper encoding/decoding of TX metadata #1159
Conversation
Signed-off-by: Akhil Repala <arepala@blinklabs.io>
Signed-off-by: Akhil Repala <arepala@blinklabs.io>
Signed-off-by: Akhil Repala <arepala@blinklabs.io>
Signed-off-by: Akhil Repala <arepala@blinklabs.io>
| ) | ||
| } else { | ||
| t.Logf("Length mismatch: original length = %d, re-encoded length = %d", len(dataBytes), len(encoded)) | ||
| } |
There was a problem hiding this comment.
We want to keep the check for the re-encoded CBOR being identical to the original decoded CBOR. If that's failing with this new code, then that should be addressed
There was a problem hiding this comment.
Sure, I will revert back the changes here
| Type() int | ||
| Cbor() []byte | ||
| Metadata() *cbor.LazyValue | ||
| Metadata() TransactionMetadataSet |
There was a problem hiding this comment.
We don't want to use TransactionMetadataSet here, only TransactionMetadatum. The Set is used at the block level to return the metadatum for all TXs included in the block.
| type TransactionMetadataSet map[uint64]TransactionMetadatum | ||
|
|
||
| type TransactionMetadatum interface { | ||
| TypeName() string |
There was a problem hiding this comment.
Add a isTransactionMetadatum() function here, so that other types can't accidentally satisfy this interface.
| TransactionBodies []ConwayTransactionBody | ||
| TransactionWitnessSets []ConwayTransactionWitnessSet | ||
| TransactionMetadataSet map[uint]*cbor.LazyValue | ||
| TransactionMetadataSet map[uint]common.TransactionMetadataSet |
There was a problem hiding this comment.
This should be TransactionMetadataSet TransactionMetadataSet. What you've got here will give you nested maps, which isn't what we want
There was a problem hiding this comment.
Do you mean TransactionMetadataSet.TransactionMetadataSet?
There was a problem hiding this comment.
Err, I meant TransactionMetadataSet common.TransactionMetadataSet
Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
WalkthroughThis PR refactors transaction metadata representation across all ledger eras from lazy CBOR values to a structured, typed Changes
Sequence Diagram(s)sequenceDiagram
participant Ledger as Ledger Layer
participant Decode as Decode Path
participant Struct as Typed Metadata
participant Encode as Encode Path
Ledger->>Decode: Raw CBOR bytes
Decode->>Decode: DecodeMetadatumRaw()
Decode->>Struct: Parse recursive structure
Struct->>Struct: Match type (Int/Bytes/Text/List/Map)
Struct-->>Encode: TransactionMetadatum
Encode->>Encode: metadatumToInterface()
Encode->>Encode: MarshalCBOR()
Encode->>Encode: Choose representation (map/array)
Encode-->>Ledger: CBOR bytes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
ledger/babbage/babbage.go (2)
353-359: Bug: address of range variable yields duplicate pointersUsing &input appends the address of the loop variable, so all entries point to the same memory (the last element). Return the value, not its address.
func (b *BabbageTransactionBody) ReferenceInputs() []common.TransactionInput { ret := []common.TransactionInput{} for _, input := range b.TxReferenceInputs.Items() { - ret = append(ret, &input) + ret = append(ret, input) } return ret }
593-599: Bug: leaking zero hash when datum is absentReturning a pointer to an empty Blake2b256 causes RPC encoding to emit 32 zero bytes instead of “no datum”. Return nil when absent.
func (o BabbageTransactionOutput) DatumHash() *common.Blake2b256 { - if o.DatumOption != nil { - return o.DatumOption.hash - } - return &common.Blake2b256{} + if o.DatumOption != nil && o.DatumOption.hash != nil { + return o.DatumOption.hash + } + return nil }ledger/conway/conway.go (1)
468-474: Bug: address of range variable yields duplicate pointersSame issue as Babbage: &input captures the loop variable address; all entries will be the same. Append the value.
func (b *ConwayTransactionBody) ReferenceInputs() []common.TransactionInput { ret := []common.TransactionInput{} for _, input := range b.TxReferenceInputs.Items() { - ret = append(ret, &input) + ret = append(ret, input) } return ret }
♻️ Duplicate comments (2)
ledger/allegra/block_test.go (1)
63-76: Reinstate exact CBOR byte-for-byte round‑trip checkWe should keep the strict equality check in addition to the structural assertions, as previously requested. If it fails, that indicates an encoding divergence we must fix rather than relaxing the test.
Apply this diff right after encoding:
@@ if encoded == nil || len(encoded) == 0 { t.Fatal("Custom encoded CBOR from AllegraBlock is nil or empty") } - // Ensure the re-encoded CBOR is structurally valid and decodes back + // Exact byte-for-byte match with original CBOR + assert.Equal(t, dataBytes, encoded, "Re-encoded CBOR must match original CBOR") + + // Ensure the re-encoded CBOR is structurally valid and decodes back var redecoded allegra.AllegraBlock if err := redecoded.UnmarshalCBOR(encoded); err != nil { t.Fatalf("Re-encoded AllegraBlock failed to decode: %v", err) }ledger/common/tx.go (1)
34-35: Seal theTransactionMetadatuminterface.Add an unexported marker (e.g.,
isTransactionMetadatum()) so unrelated types can’t accidentally satisfy the interface. This was requested previously and is still applicable.Apply this diff:
type TransactionMetadatum interface { - TypeName() string + isTransactionMetadatum() + TypeName() string } type MetaInt struct { Value int64 } +func (MetaInt) isTransactionMetadatum() {} type MetaBytes struct { Value []byte } +func (MetaBytes) isTransactionMetadatum() {} type MetaText struct { Value string } +func (MetaText) isTransactionMetadatum() {} type MetaList struct { Items []TransactionMetadatum } +func (MetaList) isTransactionMetadatum() {} type MetaPair struct { Key TransactionMetadatum Value TransactionMetadatum } type MetaMap struct { Pairs []MetaPair } +func (MetaMap) isTransactionMetadatum() {}
🧹 Nitpick comments (4)
ledger/conway/conway.go (1)
58-60: Metadata model update acknowledged; ensure CBOR optional field encodingThe switch to common.TransactionMetadataSet and appending the value directly is good. As with other eras, please verify whether the 4th element should be omitted when metadata is absent instead of appending nil, to preserve canonical CBOR and hashes.
@@ func (t *ConwayTransaction) Cbor() []byte { - if t.TxMetadata != nil { - tmpObj = append(tmpObj, t.TxMetadata) - } else { - tmpObj = append(tmpObj, nil) - } + if t.TxMetadata != nil { + tmpObj = append(tmpObj, t.TxMetadata) + }Also applies to: 521-522, 635-637, 701-703
ledger/shelley/shelley.go (1)
56-56: Type alignment looks good; consider standardizing key width.Using
map[uint]common.TransactionMetadataSetworks, but elsewhere labels useuint64. Consider switching tomap[uint64]...for consistency and to avoid platform‑dependentuintwidth.ledger/allegra/allegra.go (1)
52-53: Type alignment looks good; consider standardizing key width.
map[uint]common.TransactionMetadataSetworks; aligning onuint64across packages would remove platform‐dependent width.ledger/mary/mary.go (1)
55-56: Type alignment looks good; consider key width consistency.As with other eras, consider
map[uint64]...for cross‑platform consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
ledger/allegra/allegra.go(4 hunks)ledger/allegra/block_test.go(1 hunks)ledger/alonzo/alonzo.go(4 hunks)ledger/babbage/babbage.go(4 hunks)ledger/byron/byron.go(2 hunks)ledger/common/tx.go(3 hunks)ledger/conway/conway.go(4 hunks)ledger/mary/mary.go(4 hunks)ledger/shelley/shelley.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
ledger/byron/byron.go (1)
ledger/common/tx.go (1)
TransactionMetadataSet(41-41)
ledger/conway/conway.go (2)
ledger/common/tx.go (1)
TransactionMetadataSet(41-41)ledger/conway.go (1)
ConwayTransaction(26-26)
ledger/common/tx.go (3)
cbor/decode.go (1)
Decode(29-44)cbor/cbor.go (1)
RawMessage(36-36)cbor/encode.go (1)
Encode(27-40)
ledger/allegra/block_test.go (1)
ledger/allegra/allegra.go (2)
AllegraBlock(46-53)AllegraBlock(66-68)
ledger/mary/mary.go (1)
ledger/common/tx.go (1)
TransactionMetadataSet(41-41)
ledger/shelley/shelley.go (2)
ledger/common/tx.go (1)
TransactionMetadataSet(41-41)ledger/shelley.go (1)
ShelleyTransaction(26-26)
ledger/alonzo/alonzo.go (1)
ledger/common/tx.go (1)
TransactionMetadataSet(41-41)
ledger/allegra/allegra.go (2)
ledger/common/tx.go (1)
TransactionMetadataSet(41-41)ledger/allegra.go (1)
AllegraTransaction(25-25)
ledger/babbage/babbage.go (1)
ledger/common/tx.go (1)
TransactionMetadataSet(41-41)
🔇 Additional comments (12)
ledger/alonzo/alonzo.go (1)
58-60: Verify canonical metadata encoding against Cardano specificationThe review comment assumes "canonical transaction encoding" should omit nil metadata rather than append it. However, CBOR's canonical/deterministic rules (RFC 8949) do not mandate omitting or appending (encoding) absent/null values — that is an application- or codec-level decision.
The current implementation (appending nil) is systematic across all era files (shelley, mary, conway, babbage, alonzo, allegra at lines 714, 416, 704, 919, 770, 416 respectively), suggesting this is an intentional design choice rather than an oversight. Before refactoring, confirm that Cardano's canonical transaction encoding specification actually requires omission over nil appending. Changing the serialization format could break compatibility with existing encoded transactions if the current behavior is what the protocol expects.
ledger/babbage/babbage.go (1)
58-60: Fix CBOR encoding to omit optional metadata field instead of appending nullThe current implementation at line 919 appends
nilwhen metadata is absent, but Cardano requires omitting optional metadata fields rather than encoding them as null. This breaks canonical CBOR encoding. Apply the proposed fix:if t.TxMetadata != nil { tmpObj = append(tmpObj, t.TxMetadata) -} else { - tmpObj = append(tmpObj, nil) }Likely an incorrect or invalid review comment.
ledger/byron/byron.go (1)
145-146: Remove LazyValue concern; verify semantic mapping with Byron test dataThe LazyValue semantics concern is unfounded—ByronTransaction.Attributes is directly typed as common.TransactionMetadataSet, not lazily evaluated. However, the core concern remains valid: there is no decode/round-trip test to verify that Byron transaction attributes correctly map to the TransactionMetadataSet semantics expected by the common Transaction interface. Byron's historical "attributes" field may have different constraints than Shelley-era aux metadata, and this mapping should be validated with real Byron on-chain data.
ledger/shelley/shelley.go (3)
543-544: Good: switch from LazyValue to typed metadata set.Replacing
*cbor.LazyValuewithcommon.TransactionMetadataSetimproves type safety and enables proper encode/decode.
653-655: Accessor is consistent with new interface.Returning
common.TransactionMetadataSetmatches the updatedcommon.Transactioninterface.
711-713: CBOR path: appending the typed metadata is correct.Appending
t.TxMetadatarelies on itsMarshalCBOR()— this is the right direction and avoids double-encoding. No issues here.ledger/allegra/allegra.go (3)
242-243: Typed metadata set adoption LGTM.
352-354: Accessor matches interface change.
413-415: CBOR encoding path is correct.Using the value directly ensures
MarshalCBOR()is honored.ledger/mary/mary.go (3)
250-251: Typed metadata set adoption LGTM.
364-366: Accessor matches interface change.
413-415: CBOR encoding path is correct.
| // Tries Decoding CBOR into all TransactionMetadatum variants (int, text, bytes, list, map). | ||
| func DecodeMetadatumRaw(b []byte) (TransactionMetadatum, error) { | ||
| // Trying to decode as int64 | ||
| { | ||
| var v int64 | ||
| if _, err := cbor.Decode(b, &v); err == nil { | ||
| return MetaInt{Value: v}, nil | ||
| } | ||
| } | ||
| // Trying to decode as string | ||
| { | ||
| var s string | ||
| if _, err := cbor.Decode(b, &s); err == nil { | ||
| return MetaText{Value: s}, nil | ||
| } | ||
| } | ||
| // Trying to decode as []bytes | ||
| { | ||
| var bs []byte | ||
| if _, err := cbor.Decode(b, &bs); err == nil { | ||
| return MetaBytes{Value: bs}, nil | ||
| } | ||
| } | ||
| // Trying to decode as cbor.RawMessage first then recursively decode each value | ||
| { | ||
| var arr []cbor.RawMessage | ||
| if _, err := cbor.Decode(b, &arr); err == nil { | ||
| items := make([]TransactionMetadatum, 0, len(arr)) | ||
| for _, it := range arr { | ||
| md, err := DecodeMetadatumRaw(it) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("decode list item: %w", err) | ||
| } | ||
| items = append(items, md) | ||
| } | ||
| return MetaList{Items: items}, nil | ||
| } | ||
| } | ||
| // Trying to decode as map[uint64]cbor.RawMessage first. | ||
| // Next trying to decode key as MetaInt and value as MetaMap | ||
| { | ||
| var m map[uint64]cbor.RawMessage | ||
| if _, err := cbor.Decode(b, &m); err == nil && len(m) > 0 { | ||
| pairs := make([]MetaPair, 0, len(m)) | ||
| for k, rv := range m { | ||
| val, err := DecodeMetadatumRaw(rv) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("decode map(uint) value: %w", err) | ||
| } | ||
| if k > math.MaxInt64 { | ||
| return nil, fmt.Errorf("metadata label %d exceeds int64", k) | ||
| } | ||
| pairs = append(pairs, MetaPair{ | ||
| Key: MetaInt{Value: int64(k)}, | ||
| Value: val, | ||
| }) | ||
| } | ||
| return MetaMap{Pairs: pairs}, nil | ||
| } | ||
| } | ||
| // Trying to decode as map[string]cbor.RawMessage first. | ||
| // Next trying to decode key as MetaText and value as MetaMap | ||
| { | ||
| var m map[string]cbor.RawMessage | ||
| if _, err := cbor.Decode(b, &m); err == nil && len(m) > 0 { | ||
| pairs := make([]MetaPair, 0, len(m)) | ||
| for k, rv := range m { | ||
| val, err := DecodeMetadatumRaw(rv) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("decode map(text) value: %w", err) | ||
| } | ||
| pairs = append(pairs, MetaPair{ | ||
| Key: MetaText{Value: k}, | ||
| Value: val, | ||
| }) | ||
| } | ||
| return MetaMap{Pairs: pairs}, nil | ||
| } | ||
| } | ||
|
|
||
| return nil, errors.New("unsupported metadatum shape") | ||
| } |
There was a problem hiding this comment.
Map decoding misses valid shapes (negative int keys, byte-string keys).
CDDL allows metadatum map keys to be any metadatum (int — incl. negative, bytes, text). Current decoding only handles map[uint64]... and map[string].... Negative int keys won’t decode; byte-string keys can’t decode at all. This breaks spec compliance for some on-chain metadata.
Minimal improvement (adds negative int keys support):
@@
- // Trying to decode as map[uint64]cbor.RawMessage first.
+ // Trying to decode as map[uint64]cbor.RawMessage first.
@@
if _, err := cbor.Decode(b, &m); err == nil && len(m) > 0 {
@@
return MetaMap{Pairs: pairs}, nil
}
}
+ // Trying to decode as map[int64]cbor.RawMessage to support negative integer keys.
+ {
+ var m map[int64]cbor.RawMessage
+ if _, err := cbor.Decode(b, &m); err == nil && len(m) > 0 {
+ pairs := make([]MetaPair, 0, len(m))
+ for k, rv := range m {
+ val, err := DecodeMetadatumRaw(rv)
+ if err != nil {
+ return nil, fmt.Errorf("decode map(int) value: %w", err)
+ }
+ pairs = append(pairs, MetaPair{
+ Key: MetaInt{Value: k},
+ Value: val,
+ })
+ }
+ return MetaMap{Pairs: pairs}, nil
+ }
+ }
// Trying to decode as map[string]cbor.RawMessage first.For byte-string keys, decoding via map[[]byte]... isn’t possible in Go (non-comparable key). You likely need a streaming decode using fxamacker’s decoder to iterate map pairs as cbor.RawMessage and recursively decode key/value. Recommend a follow-up change to add that path.
📝 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.
| // Tries Decoding CBOR into all TransactionMetadatum variants (int, text, bytes, list, map). | |
| func DecodeMetadatumRaw(b []byte) (TransactionMetadatum, error) { | |
| // Trying to decode as int64 | |
| { | |
| var v int64 | |
| if _, err := cbor.Decode(b, &v); err == nil { | |
| return MetaInt{Value: v}, nil | |
| } | |
| } | |
| // Trying to decode as string | |
| { | |
| var s string | |
| if _, err := cbor.Decode(b, &s); err == nil { | |
| return MetaText{Value: s}, nil | |
| } | |
| } | |
| // Trying to decode as []bytes | |
| { | |
| var bs []byte | |
| if _, err := cbor.Decode(b, &bs); err == nil { | |
| return MetaBytes{Value: bs}, nil | |
| } | |
| } | |
| // Trying to decode as cbor.RawMessage first then recursively decode each value | |
| { | |
| var arr []cbor.RawMessage | |
| if _, err := cbor.Decode(b, &arr); err == nil { | |
| items := make([]TransactionMetadatum, 0, len(arr)) | |
| for _, it := range arr { | |
| md, err := DecodeMetadatumRaw(it) | |
| if err != nil { | |
| return nil, fmt.Errorf("decode list item: %w", err) | |
| } | |
| items = append(items, md) | |
| } | |
| return MetaList{Items: items}, nil | |
| } | |
| } | |
| // Trying to decode as map[uint64]cbor.RawMessage first. | |
| // Next trying to decode key as MetaInt and value as MetaMap | |
| { | |
| var m map[uint64]cbor.RawMessage | |
| if _, err := cbor.Decode(b, &m); err == nil && len(m) > 0 { | |
| pairs := make([]MetaPair, 0, len(m)) | |
| for k, rv := range m { | |
| val, err := DecodeMetadatumRaw(rv) | |
| if err != nil { | |
| return nil, fmt.Errorf("decode map(uint) value: %w", err) | |
| } | |
| if k > math.MaxInt64 { | |
| return nil, fmt.Errorf("metadata label %d exceeds int64", k) | |
| } | |
| pairs = append(pairs, MetaPair{ | |
| Key: MetaInt{Value: int64(k)}, | |
| Value: val, | |
| }) | |
| } | |
| return MetaMap{Pairs: pairs}, nil | |
| } | |
| } | |
| // Trying to decode as map[string]cbor.RawMessage first. | |
| // Next trying to decode key as MetaText and value as MetaMap | |
| { | |
| var m map[string]cbor.RawMessage | |
| if _, err := cbor.Decode(b, &m); err == nil && len(m) > 0 { | |
| pairs := make([]MetaPair, 0, len(m)) | |
| for k, rv := range m { | |
| val, err := DecodeMetadatumRaw(rv) | |
| if err != nil { | |
| return nil, fmt.Errorf("decode map(text) value: %w", err) | |
| } | |
| pairs = append(pairs, MetaPair{ | |
| Key: MetaText{Value: k}, | |
| Value: val, | |
| }) | |
| } | |
| return MetaMap{Pairs: pairs}, nil | |
| } | |
| } | |
| return nil, errors.New("unsupported metadatum shape") | |
| } | |
| // Tries Decoding CBOR into all TransactionMetadatum variants (int, text, bytes, list, map). | |
| func DecodeMetadatumRaw(b []byte) (TransactionMetadatum, error) { | |
| // Trying to decode as int64 | |
| { | |
| var v int64 | |
| if _, err := cbor.Decode(b, &v); err == nil { | |
| return MetaInt{Value: v}, nil | |
| } | |
| } | |
| // Trying to decode as string | |
| { | |
| var s string | |
| if _, err := cbor.Decode(b, &s); err == nil { | |
| return MetaText{Value: s}, nil | |
| } | |
| } | |
| // Trying to decode as []bytes | |
| { | |
| var bs []byte | |
| if _, err := cbor.Decode(b, &bs); err == nil { | |
| return MetaBytes{Value: bs}, nil | |
| } | |
| } | |
| // Trying to decode as cbor.RawMessage first then recursively decode each value | |
| { | |
| var arr []cbor.RawMessage | |
| if _, err := cbor.Decode(b, &arr); err == nil { | |
| items := make([]TransactionMetadatum, 0, len(arr)) | |
| for _, it := range arr { | |
| md, err := DecodeMetadatumRaw(it) | |
| if err != nil { | |
| return nil, fmt.Errorf("decode list item: %w", err) | |
| } | |
| items = append(items, md) | |
| } | |
| return MetaList{Items: items}, nil | |
| } | |
| } | |
| // Trying to decode as map[uint64]cbor.RawMessage first. | |
| // Next trying to decode key as MetaInt and value as MetaMap | |
| { | |
| var m map[uint64]cbor.RawMessage | |
| if _, err := cbor.Decode(b, &m); err == nil && len(m) > 0 { | |
| pairs := make([]MetaPair, 0, len(m)) | |
| for k, rv := range m { | |
| val, err := DecodeMetadatumRaw(rv) | |
| if err != nil { | |
| return nil, fmt.Errorf("decode map(uint) value: %w", err) | |
| } | |
| if k > math.MaxInt64 { | |
| return nil, fmt.Errorf("metadata label %d exceeds int64", k) | |
| } | |
| pairs = append(pairs, MetaPair{ | |
| Key: MetaInt{Value: int64(k)}, | |
| Value: val, | |
| }) | |
| } | |
| return MetaMap{Pairs: pairs}, nil | |
| } | |
| } | |
| // Trying to decode as map[int64]cbor.RawMessage to support negative integer keys. | |
| { | |
| var m map[int64]cbor.RawMessage | |
| if _, err := cbor.Decode(b, &m); err == nil && len(m) > 0 { | |
| pairs := make([]MetaPair, 0, len(m)) | |
| for k, rv := range m { | |
| val, err := DecodeMetadatumRaw(rv) | |
| if err != nil { | |
| return nil, fmt.Errorf("decode map(int) value: %w", err) | |
| } | |
| pairs = append(pairs, MetaPair{ | |
| Key: MetaInt{Value: k}, | |
| Value: val, | |
| }) | |
| } | |
| return MetaMap{Pairs: pairs}, nil | |
| } | |
| } | |
| // Trying to decode as map[string]cbor.RawMessage first. | |
| // Next trying to decode key as MetaText and value as MetaMap | |
| { | |
| var m map[string]cbor.RawMessage | |
| if _, err := cbor.Decode(b, &m); err == nil && len(m) > 0 { | |
| pairs := make([]MetaPair, 0, len(m)) | |
| for k, rv := range m { | |
| val, err := DecodeMetadatumRaw(rv) | |
| if err != nil { | |
| return nil, fmt.Errorf("decode map(text) value: %w", err) | |
| } | |
| pairs = append(pairs, MetaPair{ | |
| Key: MetaText{Value: k}, | |
| Value: val, | |
| }) | |
| } | |
| return MetaMap{Pairs: pairs}, nil | |
| } | |
| } | |
| return nil, errors.New("unsupported metadatum shape") | |
| } |
🤖 Prompt for AI Agents
In ledger/common/tx.go around lines 308 to 389, the current map decoding only
handles map[uint64] and map[string], missing negative integer keys and
byte-string keys; update the decode logic to also try decoding map[int64] (or
map[int64]cbor.RawMessage) to capture negative keys and convert them to MetaInt,
and add a path that uses a streaming decoder (fxamacker/cbor.Decoder) to iterate
raw map key/value pairs as cbor.RawMessage so you can recursively
DecodeMetadatumRaw on both key and value (this avoids using non-comparable
[]byte as Go map keys); return MetaMap with pairs built from the recursively
decoded key/value results and preserve existing error handling for decoding
failures.
| // Decodes the transaction metadata set. | ||
| func (s *TransactionMetadataSet) UnmarshalCBOR(cborData []byte) error { | ||
| // Trying to decode as map[uint64]cbor.RawMessage. | ||
| // Calling DecodeMetadatumRaw for each entry call to get the typed value. | ||
| { | ||
| var tmp map[uint64]cbor.RawMessage | ||
| if _, err := cbor.Decode(cborData, &tmp); err == nil { | ||
| out := make(TransactionMetadataSet, len(tmp)) | ||
| for k, v := range tmp { | ||
| md, err := DecodeMetadatumRaw(v) | ||
| if err != nil { | ||
| return fmt.Errorf("decode metadata value for index %d: %w", k, err) | ||
| } | ||
| out[k] = md | ||
| } | ||
| *s = out | ||
| return nil | ||
| } | ||
| } | ||
| // Trying to decode as []cbor.RawMessage. | ||
| // Each element in array is decoded by calling DecodeMetadatumRaw | ||
| { | ||
| var arr []cbor.RawMessage | ||
| if _, err := cbor.Decode(cborData, &arr); err == nil { | ||
| out := make(TransactionMetadataSet) | ||
| for i, raw := range arr { | ||
| var probe any | ||
| // Skipping null values as well after decoding to cbor.RawMessage | ||
| if _, err := cbor.Decode(raw, &probe); err == nil && probe == nil { | ||
| continue | ||
| } | ||
| md, err := DecodeMetadatumRaw(raw) | ||
| if err != nil { | ||
| return fmt.Errorf("decode metadata list item %d: %w", i, err) | ||
| } | ||
| out[uint64(i)] = md // #nosec G115 | ||
| } | ||
| *s = out | ||
| return nil | ||
| } | ||
| } | ||
| return errors.New("unsupported TransactionMetadataSet encoding") | ||
| } |
There was a problem hiding this comment.
Transaction metadata set should not accept array form (risk of mis-decoding).
Per CDDL, per‑transaction metadata labels are a CBOR map with non‑negative integer labels. Accepting []cbor.RawMessage here and translating indexes to labels can accept invalid shapes and blur concerns with the block‑level “auxiliary data set” (which may be array-encoded by era). Keep TransactionMetadataSet strictly map‑based; handle block‑level array shapes in the block type, not here.
Suggested change (remove array decode):
func (s *TransactionMetadataSet) UnmarshalCBOR(cborData []byte) error {
@@
- // Trying to decode as []cbor.RawMessage.
- // Each element in array is decoded by calling DecodeMetadatumRaw
- {
- var arr []cbor.RawMessage
- if _, err := cbor.Decode(cborData, &arr); err == nil {
- out := make(TransactionMetadataSet)
- for i, raw := range arr {
- var probe any
- // Skipping null values as well after decoding to cbor.RawMessage
- if _, err := cbor.Decode(raw, &probe); err == nil && probe == nil {
- continue
- }
- md, err := DecodeMetadatumRaw(raw)
- if err != nil {
- return fmt.Errorf("decode metadata list item %d: %w", i, err)
- }
- out[uint64(i)] = md // #nosec G115
- }
- *s = out
- return nil
- }
- }
return errors.New("unsupported TransactionMetadataSet encoding")
}If you need array handling for the block‑level set (tx index → per‑tx metadata), introduce a dedicated type (e.g., type BlockAuxDataSet map[uint64]TransactionMetadataSet) with its own UnmarshalCBOR that accepts both map and array forms.
📝 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.
| // Decodes the transaction metadata set. | |
| func (s *TransactionMetadataSet) UnmarshalCBOR(cborData []byte) error { | |
| // Trying to decode as map[uint64]cbor.RawMessage. | |
| // Calling DecodeMetadatumRaw for each entry call to get the typed value. | |
| { | |
| var tmp map[uint64]cbor.RawMessage | |
| if _, err := cbor.Decode(cborData, &tmp); err == nil { | |
| out := make(TransactionMetadataSet, len(tmp)) | |
| for k, v := range tmp { | |
| md, err := DecodeMetadatumRaw(v) | |
| if err != nil { | |
| return fmt.Errorf("decode metadata value for index %d: %w", k, err) | |
| } | |
| out[k] = md | |
| } | |
| *s = out | |
| return nil | |
| } | |
| } | |
| // Trying to decode as []cbor.RawMessage. | |
| // Each element in array is decoded by calling DecodeMetadatumRaw | |
| { | |
| var arr []cbor.RawMessage | |
| if _, err := cbor.Decode(cborData, &arr); err == nil { | |
| out := make(TransactionMetadataSet) | |
| for i, raw := range arr { | |
| var probe any | |
| // Skipping null values as well after decoding to cbor.RawMessage | |
| if _, err := cbor.Decode(raw, &probe); err == nil && probe == nil { | |
| continue | |
| } | |
| md, err := DecodeMetadatumRaw(raw) | |
| if err != nil { | |
| return fmt.Errorf("decode metadata list item %d: %w", i, err) | |
| } | |
| out[uint64(i)] = md // #nosec G115 | |
| } | |
| *s = out | |
| return nil | |
| } | |
| } | |
| return errors.New("unsupported TransactionMetadataSet encoding") | |
| } | |
| // Decodes the transaction metadata set. | |
| func (s *TransactionMetadataSet) UnmarshalCBOR(cborData []byte) error { | |
| // Trying to decode as map[uint64]cbor.RawMessage. | |
| // Calling DecodeMetadatumRaw for each entry call to get the typed value. | |
| { | |
| var tmp map[uint64]cbor.RawMessage | |
| if _, err := cbor.Decode(cborData, &tmp); err == nil { | |
| out := make(TransactionMetadataSet, len(tmp)) | |
| for k, v := range tmp { | |
| md, err := DecodeMetadatumRaw(v) | |
| if err != nil { | |
| return fmt.Errorf("decode metadata value for index %d: %w", k, err) | |
| } | |
| out[k] = md | |
| } | |
| *s = out | |
| return nil | |
| } | |
| } | |
| return errors.New("unsupported TransactionMetadataSet encoding") | |
| } |
| // Encodes the transaction metadata set as a CBOR map | ||
| func (s TransactionMetadataSet) MarshalCBOR() ([]byte, error) { | ||
| if s == nil { | ||
| return cbor.Encode(&map[uint64]any{}) | ||
| } | ||
| contiguous := true | ||
| var maxKey uint64 | ||
| for k := range s { | ||
| if k > maxKey { | ||
| maxKey = k | ||
| } | ||
| } | ||
| // expectedCount64 is the length the array | ||
| expectedCount64 := maxKey + 1 | ||
| if expectedCount64 > uint64(math.MaxInt) { | ||
| return nil, errors.New("metadata set too large to encode as array") | ||
| } | ||
| expectedCount := int(expectedCount64) // #nosec G115 | ||
| if len(s) != expectedCount { | ||
| contiguous = false | ||
| } else { | ||
| for i := uint64(0); i < expectedCount64; i++ { | ||
| if _, ok := s[i]; !ok { | ||
| contiguous = false | ||
| break | ||
| } | ||
| } | ||
| } | ||
| if contiguous { | ||
| arr := make([]any, expectedCount) | ||
| for i := uint64(0); i < expectedCount64; i++ { | ||
| arr[i] = metadatumToInterface(s[i]) | ||
| } | ||
| return cbor.Encode(&arr) | ||
| } | ||
| // Otherwise Encode as a map. | ||
| tmpMap := make(map[uint64]any, len(s)) | ||
| for k, v := range s { | ||
| tmpMap[k] = metadatumToInterface(v) | ||
| } | ||
| return cbor.Encode(&tmpMap) | ||
| } |
There was a problem hiding this comment.
Encoding as array when keys are contiguous can violate the CDDL for per‑tx metadata.
If labels happen to be 0..N, this will emit an array instead of the required map. That produces non‑conformant tx CBOR. For per‑tx metadata, always encode as map.
Apply this diff to always encode as map:
-func (s TransactionMetadataSet) MarshalCBOR() ([]byte, error) {
- if s == nil {
- return cbor.Encode(&map[uint64]any{})
- }
- contiguous := true
- var maxKey uint64
- for k := range s {
- if k > maxKey {
- maxKey = k
- }
- }
- expectedCount64 := maxKey + 1
- if expectedCount64 > uint64(math.MaxInt) {
- return nil, errors.New("metadata set too large to encode as array")
- }
- expectedCount := int(expectedCount64) // #nosec G115
- if len(s) != expectedCount {
- contiguous = false
- } else {
- for i := uint64(0); i < expectedCount64; i++ {
- if _, ok := s[i]; !ok {
- contiguous = false
- break
- }
- }
- }
- if contiguous {
- arr := make([]any, expectedCount)
- for i := uint64(0); i < expectedCount64; i++ {
- arr[i] = metadatumToInterface(s[i])
- }
- return cbor.Encode(&arr)
- }
- // Otherwise Encode as a map.
- tmpMap := make(map[uint64]any, len(s))
- for k, v := range s {
- tmpMap[k] = metadatumToInterface(v)
- }
- return cbor.Encode(&tmpMap)
-}
+func (s TransactionMetadataSet) MarshalCBOR() ([]byte, error) {
+ if s == nil {
+ return cbor.Encode(&map[uint64]any{})
+ }
+ tmpMap := make(map[uint64]any, len(s))
+ for k, v := range s {
+ tmpMap[k] = metadatumToInterface(v)
+ }
+ return cbor.Encode(&tmpMap)
+}📝 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.
| // Encodes the transaction metadata set as a CBOR map | |
| func (s TransactionMetadataSet) MarshalCBOR() ([]byte, error) { | |
| if s == nil { | |
| return cbor.Encode(&map[uint64]any{}) | |
| } | |
| contiguous := true | |
| var maxKey uint64 | |
| for k := range s { | |
| if k > maxKey { | |
| maxKey = k | |
| } | |
| } | |
| // expectedCount64 is the length the array | |
| expectedCount64 := maxKey + 1 | |
| if expectedCount64 > uint64(math.MaxInt) { | |
| return nil, errors.New("metadata set too large to encode as array") | |
| } | |
| expectedCount := int(expectedCount64) // #nosec G115 | |
| if len(s) != expectedCount { | |
| contiguous = false | |
| } else { | |
| for i := uint64(0); i < expectedCount64; i++ { | |
| if _, ok := s[i]; !ok { | |
| contiguous = false | |
| break | |
| } | |
| } | |
| } | |
| if contiguous { | |
| arr := make([]any, expectedCount) | |
| for i := uint64(0); i < expectedCount64; i++ { | |
| arr[i] = metadatumToInterface(s[i]) | |
| } | |
| return cbor.Encode(&arr) | |
| } | |
| // Otherwise Encode as a map. | |
| tmpMap := make(map[uint64]any, len(s)) | |
| for k, v := range s { | |
| tmpMap[k] = metadatumToInterface(v) | |
| } | |
| return cbor.Encode(&tmpMap) | |
| } | |
| // Encodes the transaction metadata set as a CBOR map | |
| func (s TransactionMetadataSet) MarshalCBOR() ([]byte, error) { | |
| if s == nil { | |
| return cbor.Encode(&map[uint64]any{}) | |
| } | |
| tmpMap := make(map[uint64]any, len(s)) | |
| for k, v := range s { | |
| tmpMap[k] = metadatumToInterface(v) | |
| } | |
| return cbor.Encode(&tmpMap) | |
| } |
🤖 Prompt for AI Agents
In ledger/common/tx.go around lines 435 to 476, the current MarshalCBOR attempts
to encode TransactionMetadataSet as an array when keys are contiguous, which can
violate the per-tx metadata CDDL; change the implementation to always encode as
a CBOR map: remove the contiguous detection and array branch, build a
map[uint64]any (tmpMap) by iterating s and converting values with
metadatumToInterface, and return cbor.Encode(&tmpMap) in all cases (handle nil s
by encoding an empty map as before).
| // converting typed metadatum back into regular go values where the CBOR library can encode | ||
| func metadatumToInterface(m TransactionMetadatum) any { | ||
| switch t := m.(type) { | ||
| case MetaInt: | ||
| return t.Value | ||
| case MetaBytes: | ||
| return []byte(t.Value) | ||
| case MetaText: | ||
| return t.Value | ||
| case MetaList: | ||
| out := make([]any, 0, len(t.Items)) | ||
| for _, it := range t.Items { | ||
| out = append(out, metadatumToInterface(it)) | ||
| } | ||
| return out | ||
| case MetaMap: | ||
| mm := make(map[any]any, len(t.Pairs)) | ||
| for _, p := range t.Pairs { | ||
| mm[metadatumToInterface(p.Key)] = metadatumToInterface(p.Value) | ||
| } | ||
| return mm | ||
| default: | ||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
Encoding maps via map[any]any can panic for byte‑string keys.
In metadatumToInterface, MetaMap builds map[any]any. If any key is MetaBytes, the resulting []byte key is not comparable and will panic at assignment. Also mixed key types could lead to undesirable encodings.
Short‑term mitigation: only build native maps when all keys are text or all keys are integers; otherwise, return an error via a custom MarshalCBOR for MetaMap that encodes pairs using the CBOR encoder directly.
Example mitigation:
func metadatumToInterface(m TransactionMetadatum) any {
switch t := m.(type) {
@@
case MetaMap:
- mm := make(map[any]any, len(t.Pairs))
- for _, p := range t.Pairs {
- mm[metadatumToInterface(p.Key)] = metadatumToInterface(p.Value)
- }
- return mm
+ // Try all-text keys
+ allText := true
+ for _, p := range t.Pairs {
+ if _, ok := p.Key.(MetaText); !ok {
+ allText = false
+ break
+ }
+ }
+ if allText {
+ mm := make(map[string]any, len(t.Pairs))
+ for _, p := range t.Pairs {
+ mm[p.Key.(MetaText).Value] = metadatumToInterface(p.Value)
+ }
+ return mm
+ }
+ // Try all-int keys
+ allInt := true
+ for _, p := range t.Pairs {
+ if _, ok := p.Key.(MetaInt); !ok {
+ allInt = false
+ break
+ }
+ }
+ if allInt {
+ mm := make(map[int64]any, len(t.Pairs))
+ for _, p := range t.Pairs {
+ mm[p.Key.(MetaInt).Value] = metadatumToInterface(p.Value)
+ }
+ return mm
+ }
+ // Fallback is handled by a dedicated MetaMap.MarshalCBOR implementation (recommended).
+ return nil
default:
return nil
}
}Recommended follow‑up: implement func (m MetaMap) MarshalCBOR() ([]byte, error) to encode map pairs via the CBOR encoder (so byte‑string keys are supported without using Go map keys).
🤖 Prompt for AI Agents
In ledger/common/tx.go around lines 478–502, the MetaMap branch currently builds
map[any]any which can panic if a key converts to []byte (non‑comparable) or mix
types; change it to first scan all pairs and detect key kinds: if every key is
MetaText convert to map[string]any, if every key is MetaInt convert to
map[int64]any (or appropriate int type) and populate with
metadatumToInterface(values); otherwise do NOT build a Go map — return the
original MetaMap value (so it can be encoded via a custom encoder) and implement
MetaMap.MarshalCBOR() to encode pairs directly with the CBOR encoder (handling
byte-string keys safely). Ensure metadatumToInterface only returns native maps
when keys are homogeneous text or integer types to avoid panics.
Closes #1136
Summary by CodeRabbit