Skip to content

Commit d302c0d

Browse files
chrisguineyclaude
andauthored
fix(blockfetch): deliver undecodable blocks to BlockRawFunc (#2186)
* fix(blockfetch): deliver undecodable blocks to BlockRawFunc handleBlock decoded every block with ledger.NewBlockFromCbor and failed the request on error, so a payload the generic type decoder cannot represent never reached BlockRawFunc. Musashi peers tag Dijkstra-shaped blocks as Conway; the strict Conway decoder rejects their 12-field Leios-extended header body, tearing down the connection before a consumer that understands the layout could decode it. When the raw callback is the consumer that would receive the block, a decode failure now falls back to raw delivery. Range correlation still applies: its inputs are read straight from the block header CBOR, which is era-agnostic for the fields involved, so a peer cannot inject an unrelated block by sending one the type decoder rejects. recordBlock takes a point and previous hash rather than a decoded block to serve both paths. Every other consumer is unchanged and still fails on an undecodable block: GetBlock, BlockFunc, and the block pipeline, which takes precedence over BlockRawFunc and runs its own typed decode. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018DcqSoDCsANbPVamS3iuYU Signed-off-by: Chris Guiney <chris@guiney.net> * fix(blockfetch): guard the failed-decode block against nil dereference nilaway flagged the decoded block as dereferenced without a guard: the restructure that added the raw fallback moved the deref away from the `decodeErr != nil` return that had guarded it. Adding the guard exposed a real panic behind it. The era decoders return a typed nil pointer alongside their error, so the interface holding one is not itself nil and passes `block != nil`; every method call on it then panics. Normalize a failed decode to an untyped nil so the raw path is taken, and reject a decoder that returns neither a block nor an error rather than handing nil to a consumer. Also assert the CBOR element counts in the range test helper before indexing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018DcqSoDCsANbPVamS3iuYU Signed-off-by: Chris Guiney <chris@guiney.net> * fix(blockfetch): keep the raw fallback out of validation verdicts The fallback treated every decode failure as an unrepresentable layout, but that set includes blocks the decoder read and then rejected. With validation enabled, which is the default, a peer could keep a valid header and replace the body: the typed decode failed on the body hash, header-only correlation still passed, and the tampered bytes reached BlockRawFunc. Gate the fallback on whether the layout is representable at all, by decoding once with validation disabled. A block that then decodes was understood and refused, so the refusal stands; one that still fails could not be read, which is the case the raw callback exists for. Keying on that rather than on the error type covers the whole class: the body-hash checks return a typed common.ValidationError, but the nil-header checks beside them return a plain error, and both are now refused. Reported by CodeRabbit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018DcqSoDCsANbPVamS3iuYU Signed-off-by: Chris Guiney <chris@guiney.net> * test(blockfetch): cover the origin prev_hash branch and await shutdown The agreement test claimed to exercise an origin prev_hash, but an encoded zero Blake2b256 is a 32-byte bytestring, not CBOR null, so the null branch in decodeRawPrevHash was never reached. Add a case that rewrites prev_hash to null; it fails without that branch. runTestExpectingError checked for leaked goroutines straight after Close, which only starts teardown, so the error-path tests could report a connection still unwinding as a leak. Wait for shutdown first, as runTest does. Reported by Cubic. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018DcqSoDCsANbPVamS3iuYU Signed-off-by: Chris Guiney <chris@guiney.net> --------- Signed-off-by: Chris Guiney <chris@guiney.net> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent e3354f1 commit d302c0d

4 files changed

Lines changed: 1142 additions & 13 deletions

File tree

protocol/blockfetch/client.go

Lines changed: 70 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1100,22 +1100,78 @@ func (c *Client) handleBlock(msgGeneric protocol.Message) error {
11001100
fmt.Errorf("%s: decode error: %w", ProtocolName, err),
11011101
)
11021102
}
1103-
block, err := ledger.NewBlockFromCbor(
1103+
block, decodeErr := ledger.NewBlockFromCbor(
11041104
wrappedBlock.Type,
11051105
wrappedBlock.RawBlock,
11061106
lcommon.VerifyConfig{
11071107
SkipBodyHashValidation: c.config.SkipBlockValidation,
11081108
},
11091109
)
1110-
if err != nil {
1111-
return c.failRequest(req, err)
1110+
if decodeErr != nil {
1111+
// The era decoders return a typed nil pointer alongside their error
1112+
// (see conway.NewConwayBlockFromCbor), and an interface holding one
1113+
// is not itself nil, so a `block != nil` check would pass and every
1114+
// method call on it would panic. Normalize to an untyped nil so a
1115+
// failed decode cannot be mistaken for a usable block below.
1116+
block = nil
1117+
}
1118+
// A raw callback decodes the payload itself, so a generic type decoder
1119+
// that cannot represent the full wire layout must not fail the request
1120+
// before the callback ever runs. This is how a block whose era tag
1121+
// understates its actual shape -- a Dijkstra-shaped block tagged Conway,
1122+
// say -- reaches a consumer that does understand it.
1123+
//
1124+
// A block the decoder rejected on validation is excluded: it was
1125+
// representable, so the verdict stands and must not be routed around
1126+
// (see blockLayoutRepresentable). What remains is the layout it could
1127+
// not read at all.
1128+
//
1129+
// The rest of the condition is exactly "the raw callback is the consumer
1130+
// that would receive this block", matching the delivery decisions made
1131+
// below. Every other consumer wants a decoded block and keeps failing
1132+
// here: GetBlock, which hands its caller a ledger.Block; BlockFunc; and
1133+
// the block pipeline, which takes precedence over BlockRawFunc and does
1134+
// its own typed decode (pipeline.decodeStage).
1135+
rawFallback := decodeErr != nil &&
1136+
req.delivery == deliveryCallback &&
1137+
c.config.Pipeline == nil &&
1138+
c.config.BlockRawFunc != nil &&
1139+
!blockLayoutRepresentable(wrappedBlock.Type, wrappedBlock.RawBlock)
1140+
if decodeErr != nil && !rawFallback {
1141+
return c.failRequest(req, decodeErr)
1142+
}
1143+
if decodeErr == nil && block == nil {
1144+
// Not reachable through any current decoder, but the rest of this
1145+
// function hands the block to a consumer, so state the contract
1146+
// rather than relying on it.
1147+
return c.failRequest(req, fmt.Errorf(
1148+
"%s: block decoder returned no block and no error",
1149+
ProtocolName,
1150+
))
1151+
}
1152+
// Range correlation applies either way. Without a decoded block its
1153+
// inputs are read straight from the block header CBOR, so a peer still
1154+
// cannot inject an unrelated block into a valid batch by sending one the
1155+
// type decoder rejects.
1156+
var blockPoint pcommon.Point
1157+
var prevHash []byte
1158+
if block != nil {
1159+
blockPoint = pcommon.NewPoint(
1160+
block.SlotNumber(),
1161+
block.Hash().Bytes(),
1162+
)
1163+
blockPrevHash := block.PrevHash()
1164+
prevHash = blockPrevHash.Bytes()
1165+
} else {
1166+
info, err := rawBlockHeaderInfoFromCbor(wrappedBlock.RawBlock)
1167+
if err != nil {
1168+
return c.failRequest(req, errors.Join(decodeErr, err))
1169+
}
1170+
blockPoint = info.point
1171+
prevHash = info.prevHash
11121172
}
1113-
blockPoint := pcommon.NewPoint(
1114-
block.SlotNumber(),
1115-
block.Hash().Bytes(),
1116-
)
11171173
c.queueMutex.Lock()
1118-
err = req.recordBlock(block, blockPoint)
1174+
err = req.recordBlock(blockPoint, prevHash)
11191175
c.queueMutex.Unlock()
11201176
if err != nil {
11211177
return c.failRequest(req, err)
@@ -1217,11 +1273,13 @@ func pointsEqual(a, b pcommon.Point) bool {
12171273

12181274
// recordBlock validates and records the next block in a requested inclusive
12191275
// range. BlockFetch does not carry the expected interior points on the wire,
1220-
// so continuity is established from each decoded block's previous hash. The
1221-
// caller must hold queueMutex.
1276+
// so continuity is established from each block's previous hash. It takes the
1277+
// point and previous hash rather than a block so it applies equally to a
1278+
// block delivered raw, whose typed decode is the consumer's job. The caller
1279+
// must hold queueMutex.
12221280
func (req *rangeRequest) recordBlock(
1223-
block ledger.Block,
12241281
blockPoint pcommon.Point,
1282+
prevHash []byte,
12251283
) error {
12261284
if !pointInRange(blockPoint, req.start, req.end) {
12271285
return fmt.Errorf(
@@ -1257,8 +1315,7 @@ func (req *rangeRequest) recordBlock(
12571315
req.lastPoint.Slot,
12581316
)
12591317
}
1260-
prevHash := block.PrevHash()
1261-
if !bytes.Equal(prevHash[:], req.lastPoint.Hash) {
1318+
if !bytes.Equal(prevHash, req.lastPoint.Hash) {
12621319
return fmt.Errorf(
12631320
"%s: received block does not follow previous range point: slot=%d hash=%x previous_hash=%x",
12641321
ProtocolName,

0 commit comments

Comments
 (0)