Skip to content

Serialize node block in the requested form, stripping witness. - #1069

Open
echennells wants to merge 1 commit into
libbitcoin:masterfrom
echennells:fix-block-serialize-non-witness
Open

Serialize node block in the requested form, stripping witness.#1069
echennells wants to merge 1 commit into
libbitcoin:masterfrom
echennells:fix-block-serialize-non-witness

Conversation

@echennells

Copy link
Copy Markdown
Contributor

A peer requesting a block without witness (getdata MSG_BLOCK) crashes the node.

messages::block held the store's wire bytes plus an advisory witnessed_ flag, and serialize/size guarded on witness == witnessed_. The generic serializer calls serialize with the default witness = true, so the guard fails, serialize returns false, and the null payload is sent without a null check — any peer can trigger it (in debug the size() assert trips first).

The guard is the root error, but the witness argument must also function: a witness node servicing a non-witness peer must strip the witness on serialize, and the serializer cannot rely on the held object's form, because one object may be sent to peers with differing requirements.

The message now holds a system::chain::block_view instead of the wire bytes and delegates serialize/size to block.to_data(witness) / block.serialized_size(witness) — the view strips the witness when serialized without it. protocol_block_out_106 selects the form at read time. The incorrect guard and assert are removed.

messages::transaction carries the same guard, but nothing serializes it yet (transaction-out re-serializes from the parsed transaction), so it is left unchanged.

Depends on libbitcoin/libbitcoin-system#1905 — will not build without it.

Full node test suite green.

@echennells
echennells force-pushed the fix-block-serialize-non-witness branch 2 times, most recently from 627e27c to 05a1706 Compare July 7, 2026 02:32
@evoskuil

Copy link
Copy Markdown
Member

Looks like this needs rebase.

A peer requesting a block without witness (getdata MSG_BLOCK) crashes the
node. messages::block held the store's wire bytes plus an advisory witnessed_
flag, and serialize/size guarded on witness == witnessed_. The generic
serializer calls serialize with the default witness = true, so the guard
fails, serialize returns false, and the null payload is sent without a null
check. Any peer can trigger it.

The guard is the root error, but the witness argument must also function: a
witness node servicing a non-witness peer must strip the witness on serialize,
and the serializer cannot rely on the held object's form because one object
may be sent to peers with differing requirements. Hold a
system::chain::block_view instead of raw bytes and delegate serialize and size
to block.to_data(witness) and block.serialized_size(witness); the view strips
the witness when serialized without it. protocol_block_out_106 selects the
form at read time via get_wire_block(link, witness) and wraps the result in the
view.

messages::transaction carries the same guard but nothing serializes it yet
(transaction-out re-serializes from the parsed transaction), so it is left
unchanged.

Depends on the libbitcoin-system block_view and transaction_view to_data
implementation.
@echennells
echennells force-pushed the fix-block-serialize-non-witness branch from 05a1706 to 9bde3f0 Compare August 6, 2026 21:07
@echennells

Copy link
Copy Markdown
Contributor Author

Rebased onto master (ada11dc7). The change itself is unchanged — range-diff against the prior head is identical and the patch-id matches; only the base moved.

The previous CI red was the stale base rather than this change: the old head predated the database::store template signature change, so node no longer compiled against current database master (define.hpp:60: type/value mismatch at argument 1 in template parameter list for 'template<template<size_t...> class Storage> class database::store'). The system dependency this PR needs (#1905, block_view::to_data) merged on 2026-07-09.

Re-verified on a from-source build against current deps (system 35b3e4c5, database a69f1c96, network 41caed22), two binaries differing only in the node tree:

  • Serving a legacy getdata MSG_BLOCK on master node SIGSEGVs — messages::block::serialize returns false for the non-witness form, and the resulting null payload is dereferenced in channel_peer::send. With this change the block is served.
  • Real testnet3 block 5,103,550 (Core: 2438 B witness, 1309 B stripped): block_view::to_data(false) = 1309 B and is byte-equal to chain::block::to_data(false); to_data(true) is byte-identical to the stored wire block. Same result end-to-end over the P2P wire — MSG_BLOCK 1309 B, MSG_WITNESS_BLOCK 2438 B.

This build is currently running as a live testnet3 node at tip.

CI has not picked up the new head yet — it may need an approve-and-run.

@echennells

Copy link
Copy Markdown
Contributor Author

CI is green on the rebased head (14/14). Three jobs failed on the first attempt — two cmake release configs and the macOS gnu job — but that run was cancelled mid-flight; they pass on re-run with no change to the head.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants