Add not_found protocol overrides at bip37. - #1106
Conversation
There was a problem hiding this comment.
The correct implementation is more invasive than implemented, as required by the design. Each versioned/negotiated protocol level implies a class to encapsulate the behavior, even if the behavior is optional at that level. This keeps protocol semantics isolated. The vtable cost is negligible, while the organizational advantage is significant. Otherwise we would end up with a perpetually-growing list of conditions on a single protocol class.
| const bool node_pruned_; | ||
| const bool node_witness_; | ||
| const bool allow_overlapped_; | ||
| const bool not_found_allowed_; |
There was a problem hiding this comment.
Other configurable optional p2p protocol features:
[peer]
enable_alert = false # send/log alert messages (no send option)
enable_reject = false # send/log reject messages
enable_address = true # in/out address messages
enable_address_v2 = false # in/out address v2 messages
enable_witness_tx = false # in/out wtxid for tx announcements
enable_compact = false # in/out compact block announcements
enable_relay = false # in/out transaction announcements
enable_privacy = true # opportunistic encryption (bip324)
There was a problem hiding this comment.
If this is to be an option (as opposed to version-based), which it should be, then it must be routed through config and parse.
| node_witness_(session->node_settings().provide_witness), | ||
| allow_overlapped_(session->node_settings().allow_overlapped), | ||
| not_found_allowed_(negotiated_version() >= | ||
| network::messages::peer::not_found::version_minimum), |
There was a problem hiding this comment.
This is not consistent with protocol versioning design. See existing patterns for adding features at distinct protocol levels. This requires protocol class derivation at the protocol level in which the feature is activated, and then attachment of the derived protocol in the case where it is active.
| } | ||
|
|
||
| // A limited node answers for a block it has pruned, and remains unfaulted. | ||
| struct p2p_limited_setup_fixture |
There was a problem hiding this comment.
Don't add ad-hoc inline fixtures, there is a dedicated text cpp/hpp for this (and established pattern in server).
| // ---------------------------------------------------------------------------- | ||
|
|
||
| // This protocol is also attached below bip130, and when headers-first is | ||
| // disabled, so the peer may be below bip37, where not_found is undefined. |
|
|
||
| // This tx could not have been advertised to the peer. | ||
| stop(system::error::not_found); | ||
| // The protocol is attached above bip37, where not_found is defined. |
ffa2c08 to
0f6b385
Compare
echennells
changed the title
2 participants
The node stops the channel when a peer requests a block or transaction it cannot serve. bitcoind replies
notfoundand stays connected. Implements #986.Reworked per review: the behaviour is a protocol level, not a condition.
protocol_block_out_70001andprotocol_transaction_out_70001derive from the_106classes and overridehandle_unservable. The_106classes retain the stop, asnot_foundis undefined below bip37, andprotocol_block_out_70012now derives from_70001. The session attaches the derived protocol when bip37 is negotiated.The reply is gated by
peer.enable_not_found, routed through config and parse alongside the other optional peer features.A hash resolving to no header is answered before the checkpoint height query, which faults the store on a terminal link.
Six cases cover unknown, pruned and unassociated blocks, an unknown transaction, a peer below bip37, and the option disabled. Fixtures live in
p2p_setup_fixture.Depends on libbitcoin/libbitcoin-network#879.