Serialize node block in the requested form, stripping witness

[echennells]

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 can't 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 raw 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 via get_wire_block(link, witness) and wraps the result in the view. 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's left unchanged.

Depends on echennells/libbitcoin-system#2 (the block_view / transaction_view to_data implementation) — won't build standalone. Full node test suite green.

[evoskuil]

The bug looks correct, but the resolution is based on a false premise. The store holds what the node gives it, and the node can be configured to be a witness node or not (based on configured service bits, which as user config can change, even though such a change doesn't probably make a lot of sense it's not disallowed.

As a witness node the protocols allow the peer to request witness or non-witness data, and expect the peer to provide witness data as necessary for validation, and will be dropped for protocol non-compliance if the validation fails. As a non-witness node, peers making witness requests/submissions are dropped for protocol non-compliance.

The store doesn't care, nor do the serializers. We cannot assume that these are always aligned. That's a hardwired assumption in other nodes, not libbitcoin. So a valid block/tx serializes to the store with the witness it if has it, otherwise without - whatever the channel allowed to be passed on. But... a witness node with a channel servicing a non-witness peer (possible in the above scenario) requires that the node respond to the peer with the requested serialization, which can be a downgrade from what the store retains and possibly, from the object being serialized.

The serializer does not know where the block came from, and it's modeled after serialization of a whole object, potentially with the need to strip it. This is because an object may be broadcast across multiple peers, with varying serialization requirements based on negotiated protocol version and service flags. That's why these values are passed to serialization. Relying on the underlying object would be a break to the serializer.

So the proper resolution is to allow the object to pass serialization as a non-witness (whether it's actually a witness tx or not) but to cause the serialization to strip the witness if the underlying object does in fact have a witness - which again, the serializer doesn't know). We wouldn't retain these parameters as "advisory", they have a function.

The block and tx views are designed to retain a witness serialization and present it as witnessed or not (stripped) based on parameterization. It the parameter is witness false and there is no witness, the stripped (or pre-stripped) data is returned. If the parameter is witness true and there is no witness, the presumption is that the tx actually has no witness, and the data is returned. If the parameter is witness true and there is a witness, the unstripped data is returned.

The guards (assert/compares) in the current impl are incorrect however, which is the root error.

[evoskuil]

This is making a LOT more sense. I haven't traced the minutia, but it's def the right approach. The build breaks are from an intermediate broken build.

[evoskuil]

Can you get this rebased and green so that we can merge?