BIP Draft: Stale Tip Relay - #2241
Conversation
edilmedeiros
left a comment
There was a problem hiding this comment.
Thanks for submitting your proposal. Gave a round of review, some comments are about the technique itself, but others are more editorial to try to improve and clarify the specification.
|
|
||
| This BIP defines a new [BIP 434][BIP434] feature id ("the `staletip` feature"): | ||
|
|
||
| * `https://github.com/w0xlt/bitcoin/tree/staletip-v4` |
There was a problem hiding this comment.
This can be left to the reference implementation section.
There was a problem hiding this comment.
The URL here is the temporary experimental featureid, not just a reference implementation link. BIP 434 requires an unassigned experimental feature to use a globally unique identifier, such as its repository URL. I’ll clarify that distinction and move the standalone implementation link to the Reference Implementation section.
For features published as a BIP, the featureid SHOULD be the assigned BIP number, eg "BIP434", or be based on the BIP number (eg, "BIP434v2" where the "v2" suffix covers versioning, or "BIP434.3" where the ".3" suffix covers part 3 of the BIP). For experimental features that do not (yet) have a BIP number assigned, some other unique identifier MUST be chosen, such as a URL to the repository where development is taking place, or the sha256 digest of some longer reference.
| If this specification is assigned a BIP number, the feature id SHOULD be updated | ||
| to a BIP-number based identifier as recommended by BIP 434, for example | ||
| `BIPxxx` or `BIPxxxv1`. |
There was a problem hiding this comment.
It's weird to expect an specification proposal might not get a number… Why not specify BIPxxx for now an update when a number is assigned, without the conditional?
There was a problem hiding this comment.
Actually, this gets confusing here: the feature is called staletip everywhere, but the serialization in BIP434 featureid part of the message should be BIPxxx?
There was a problem hiding this comment.
staletip is the descriptive feature name and P2P message command. It isn’t the serialized BIP 434 featureid. The experimental implementation currently uses the repository URL as its globally unique featureid, and the published specification will the BIP number. I’ll make that distinction explicit.
| The `staletip` feature data MUST contain at least one byte. The first byte is a | ||
| boolean, `prefers_blocks`, indicating whether the node advertising the feature | ||
| prefers to collect the full block data associated with stale tips: | ||
|
|
||
| * `\x00`: the node prefers header-only announcements and does not request that | ||
| announcements be delayed until block data is available. | ||
| * `\x01`: the node prefers announcements that include availability of block | ||
| data where practical. | ||
|
|
||
| Nodes receiving an empty `staletip` feature data field, or a first byte other | ||
| than `\x00` or `\x01`, MUST ignore that peer's `staletip` feature | ||
| advertisement. They SHOULD NOT disconnect solely because the feature data is not | ||
| understood. | ||
|
|
||
| For future compatibility, nodes MUST ignore any additional feature data bytes | ||
| after the first byte. |
There was a problem hiding this comment.
Since the feature is versioned, this slack is really needed? I mean, why not accept EXACTLY one byte with the two expected values?
There was a problem hiding this comment.
Good point :). BIP 434’s versioned feature IDs give us a clean way to make future changes. I’ll update the specification to require one byte with value 0x00 or 0x01.
| - Nodes SHOULD ignore any headers found to be invalid, and SHOULD NOT disconnect | ||
| or otherwise punish peers for relaying invalid headers[^rat-ignoreinvalid]. |
| - Nodes SHOULD ignore any headers found to be invalid, and SHOULD NOT disconnect | ||
| or otherwise punish peers for relaying invalid headers[^rat-ignoreinvalid]. | ||
| - If `have_block` is `true`, nodes that prefer to collect the full block data | ||
| SHOULD request missing block data for the announced stale branch in the normal |
There was a problem hiding this comment.
SHOULD request missing block data for the announced stale branch in the normal
Sounds that MAY would work better here. What if I already have that specific stale block, SHOULD I request it again?
There was a problem hiding this comment.
What if I already have that specific stale block
Spec says "SHOULD request missing block data". If the node already has that stale block, its data is not missing and should not be requested again.
| [^rat-compressedheader]: Omitting the previous block hash from each header saves | ||
| 32 bytes per header (40%), as this field can be reconstructed from the | ||
| preceding headers in the message. This does not apply to the first header, | ||
| which is why the fork point must be included explicitly. This BIP does not | ||
| attempt to omit `nBits` or compress `nTime` or `nVersion`, because | ||
| reconstructing those fields is significantly more complicated for | ||
| comparatively much less potential gain. |
There was a problem hiding this comment.
It assumes there's only one stale branch, which is probably not true in testnet and may even happen in mainnet. The design can deal with more than one competing branch?
There was a problem hiding this comment.
Each staletip message describes one linear branch. If there are multiple competing branches, the sender will have to send a separate message for each one. I’ll clarify this in the spec.
| #### Reconstructing Headers | ||
|
|
||
| Headers may be reconstructed from a `staletip` message via the following | ||
| algorithm: | ||
|
|
||
| ``` | ||
| std::vector<CBlockHeader> headers; | ||
| uint256 prev_hash = staletip_msg.fork_point; | ||
| for (const auto& ch : staletip_msg.headers) { | ||
| headers.emplace_back({ | ||
| .nVersion = ch.version, | ||
| .hashPrevBlock = prev_hash, | ||
| .hashMerkleRoot = ch.merkle_root, | ||
| .nTime = ch.time, | ||
| .nBits = ch.bits, | ||
| .nNonce = ch.nonce, | ||
| }); | ||
| prev_hash = headers.back().GetHash(); | ||
| } | ||
| ``` | ||
|
|
||
| Note that headers are reconstructed in order, from oldest (closest to the | ||
| `fork_point`) to newest (the stale tip itself). | ||
|
|
There was a problem hiding this comment.
Not sure this is necessary and potentially implies maintaining this code in the specification over time.
There was a problem hiding this comment.
I think having the reconstruction algorithm in the specification is helpful. That said, it doesn’t need to be C++, so I’ve replaced it with pseudocode.
|
Thanks for opening this here. Plan to review! |
- Move the staletip message definition before negotiation details - Require an exact one-byte feature payload and clarify the BIP 434 feature ID. - Define branch ordering and handling of competing branches - Replace implementation-specific reconstruction code with pseudocode, - Clarify denial-of-service handling.
5b30d29 to
d055be1
Compare
@edilmedeiros Thank you so much for taking the time to review. Rebased and added a commit (d055be1) to address Edil's feedback. |
This BIP is a specification for a new an opt-in P2P message,
staletip, for relaying recent stale tips between peers.A proof-of-concept implementation is available at https://github.com/w0xlt/bitcoin/tree/staletip-v4.
Mailing list post - https://gnusha.org/pi/bitcoindev/d92f1615-368b-4406-b326-a1799c72a555n@googlegroups.com/
Discussion - https://groups.google.com/g/bitcoindev/c/AwOPNxF15mU
Feedback is welcome.