Send not_found for unservable get_data. - #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. |
2 participants
The node stops the channel when a peer requests a block or transaction that it cannot serve. bitcoind replies
notfoundand stays connected. Implements #986.The node now replies and resumes the send loop at four sites: a hash that resolves to no header, a pruned block, a block that cannot be read from the archive, and an absent transaction. Each reply is sent from the send completion handler, as a block is, so nothing is produced until the prior write completes.
The hash that resolves to no header is tested first, because the checkpoint height query faults the store on a terminal link. Such a hash is ordinary peer input, so the query is no longer reached with one.
protocol_block_out_106is attached without a version condition, so its peer may be below bip37, wherenot_foundis undefined. It captures the negotiated level at construction and retains the existing stop below that level.protocol_transaction_out_106is attached only above bip37, so the level is asserted there.A request is already bounded by
max_inventoryat deserialization, so no additional limit is imposed.Four cases cover the block sites, including a peer below bip37. The transaction site awaits a fixture that negotiates relay.