Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BIP 331: Ancestor Package Relay #1382

Merged
merged 1 commit into from
Apr 24, 2024

Conversation

glozow
Copy link
Member

@glozow glozow commented Oct 13, 2022

@luke-jr luke-jr changed the title Ancestor Package Relay BIP 331: Ancestor Package Relay Oct 21, 2022
@luke-jr
Copy link
Member

luke-jr commented Oct 21, 2022

Assigned BIP # 331

@glozow
Copy link
Member Author

glozow commented Oct 24, 2022

Updated for number

bip-0331.mediawiki Outdated Show resolved Hide resolved
@naumenkogs
Copy link
Member

naumenkogs commented Dec 23, 2022

I'm not a big mempool expert, but this seems like a good direction.


  1. There are obvious typos and misalignments you're probably aware of, i assume we shouldn't merge them?

Package Information Only

This example seems confusing: it violates the following rule A node MUST NOT send a "ancpkginfo" message that has not been requested by the recipient. Upon receipt of an unsolicited "ancpkginfo", a node may disconnect the sender.
It will likely work if you expand the example to 3 transactions?

  1. Unclear what to do when transactions are replaced in the middle of this process. Should that be covered? Probably will become clearer when you prototype.

  2. Same about timestamping concerns i guess.

  3. As a style comment, I'd suggest doing some compression of the text :)

  4. Is Package Erlay possible?
    6a) I actually made some attempt to handle parent/child within Erlay, do you think we should exercise the overlap between the two better? E.g., for those packages flowing through the network (not the IBD case, but fee-bump or something) Erlay might do the batching you need.
    6b) One might also think about "what if we reconcile packages"? Probably bad idea, because the sets (packages) are too small? My point is that this should not be confused with what you describing.

  5. MSG_ANCPKGINFO and getpkgtxns have some redundancy in the data... The latter could refer to the hash of the former + the indexes, for example? Not sure savings worth the complexity (how often this will be used? every lightning closure?) but maybe worth making it into q/a :)

@glozow
Copy link
Member Author

glozow commented Jan 9, 2023

Thanks for the review @naumenkogs! yes, I think it's best to talk specifics with the actual implementation but I'll throw some quick answers for now:

Unclear what to do when transactions are replaced in the middle of this process. Should that be covered? Probably will become clearer when you prototype.

One should treat this situation the same way we do for transactions that get replaced in between inv and getdata (I don't see any reason it should be different). Currently, we send "notfound". If in the future we decide to relay recently-replaced transactions, then package relay would include recently-replaced transactions as well. Though I agree this seems more like an implementation question than a spec question. In the spec, I would leave it as "the receiver may respond with notfound."

I actually made some attempt to handle parent/child within Erlay, do you think we should exercise the overlap between the two better? E.g., for those packages flowing through the network (not the IBD case, but fee-bump or something) Erlay might do the batching you need.

Yes it's great that Erlay orders them correctly. Note below-fee-filter transactions to the reconciliation bucket so not every case is handled. I plan to have a rebase-on-top-of-Erlay version so we can exercise the two together.

One might also think about "what if we reconcile packages"? Probably bad idea, because the sets (packages) are too small? My point is that this should not be confused with what you describing.

Mostly the "Is Package Erlay possible" section was to clarify that they aren't mutually exclusive.
Reconciling packages is I think something that may come up when we get to sender-initiated package relay. I'm thinking, after reconciliation, it may be useful to send a package inv instead of individual tx invs if there are fee-bumps... But for now, since packages must be requested by the receiver, we don't need to think about that yet.

MSG_ANCPKGINFO and getpkgtxns have some redundancy in the data... The latter could refer to the hash of the former + the indexes, for example?

Yeah that's an interesting idea, we could do a bitfield of 4 bytes (should cover any package since max count is 25). However this means you can only request getpkgtxns from somebody who sent an ancpkginfo (currently this is not a requirement). It also requires the sender to keep track of (allocate memory for, remember to expire) what they announced so they know what "the ith transaction from this package" means. Thoughts?

@naumenkogs
Copy link
Member

Though I agree this seems more like an implementation question than a spec question. In the spec, I would leave it as "the receiver may respond with notfound."

The thing with current notfound is that it's atomic I guess — makes it easy to consider every announcement independent and our code treats it as such (in a poor way but there are also no expectations). With packages, I imagine a CPFP transaction gets replaced and dropped from the package due. A package recipient would then do what, drop the entire package, or consider it invalid?

A requestor will get this package which doesn't pass the fees, and drop it on the floor? Or should it re-request the package? At the moment, I'm not interested in the final answer — but interested in making sure we don't miss such questions.

Reconciling packages is I think something that may come up when we get to sender-initiated package relay. I'm thinking, after reconciliation, it may be useful to send a package inv instead of individual tx invs if there are fee-bumps... But for now, since packages must be requested by the receiver, we don't need to think about that yet.

Not sure this anything to do with who initiates packages. Let's think about the current proposal. Imagine, upon the announcement of C, the receiver sends getdata(MSG_ANCPKGINFO, C), along with some compressed data saying that he already has 1 parent and 1 child of C (by just looking at the wtxid). Then it would save us announcing these 2 wtxids in the reverse direction.

Yeah that's an interesting idea, we could do a bitfield of 4 bytes (should cover any package since max count is 25). However this means you can only request getpkgtxns from somebody who sent an ancpkginfo (currently this is not a requirement). It also requires the sender to keep track of (allocate memory for, remember to expire) what they announced so they know what "the ith transaction from this package" means. Thoughts?

I don't think memory allocation and stuff is a show-stopper, but it might be too much complexity if the gain is low. We should measure how much savings it allows... I think saving 28 byte per announced tx might be worthy. As for the "same peer", do you ever expect packages to be requested from a different peer from the one announced them?

bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
@glozow
Copy link
Member Author

glozow commented Mar 28, 2023

@jnewbery thanks, took all suggestions

CI seems to be failing on bip-0327 title being too long, so will ignore for now.

@kallewoof
Copy link
Contributor

Sorry about that, the BIP-327 title issue has been fixed, so please rebase.

bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved

A "combined hash" serves as a unique "package id" for some list of unique transactions.

The combined hash of a package of transactions is equal to the hash of each transaction's wtxid, concatenated in lexicographical order.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use a BIP340 tagged hash here? (I'm not sure, thinking out loud). For example, could be relevant for these rolling bloom filters that contain hashes corresponding to both txes and packages?

bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
@glozow
Copy link
Member Author

glozow commented Apr 5, 2023

Thanks @jnewbery @stickies-v @willcl-ark for the feedback. I'm working on incorporating the suggestions, apologies for the delay.

@glozow
Copy link
Member Author

glozow commented Apr 22, 2023

Changed the version field in "sendpackages" to versions, now interpreted as a bitfield, and updated the negotiation procedure accordingly. (#1382 (comment))
Swapped the inv values for MSG_ANCPKGINFO and MSG_PKGTXNS (#1382 (comment))

Neither of these are implemented in the branch I had linked before, so I've removed that link. Will update when I've done that.

I've restructured the BIP to make the difference between "generic" (can be used in future versions of package relay) vs "ancestor info" (first defined version) parts more clear (#1382 (comment)). Also updated the diagrams and took wording suggestions.

@fjahr
Copy link
Contributor

fjahr commented May 1, 2023

Maybe it's obvious or I am missing something but I couldn't see the answer in the doc: How would a node decide between sending Inv(PKGINFO, C) or Inv(WTX, C) in the Generic Package Relay example graphic? Is it always sending PKGINFO if the peer wants packages and C is part of a package?

@instagibbs
Copy link
Member

@fjahr "The inv type should not be used in announcements" it's a type used for getdata only IIUC

@fjahr
Copy link
Contributor

fjahr commented May 2, 2023

@fjahr "The inv type should not be used in announcements" it's a type used for getdata only IIUC

Hmm, ok, first insight: the inconsistencies in the message naming can be confusing. I guess it gets obvious when someone goes deeper but I did a pretty superficial pass and then tried to find an answer to this question and I guess I stumbled over that it's PKGINFO in the graphics but ANCPKGINFO in the text.

It seems that the name has changed before which makes researching past discussions on the mailing list harder. Maybe that could be noted somewhere (the artist formerly known as PKGINFO, also formerly know as MSG_PCKG1 etc.). Not sure it's worth the hassle but in this case it would have helped me.

In an old ML post I found:

''Q: Under what circumstances should a sender announce a
child-with-unconfirmed-parents package?''

A child-with-unconfirmed-parents package for a transaction should be
announced when it meets the peer's fee filter but one or more of its
parents don't; a "inv(MSG_PCKG1)" instead of "inv(WTX)" should be sent
for the child. Each of the parents which meet the peer's fee filter
should still be announced normally.

(from https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-May/020493.html)

But this is now changed it seems so maybe the graphics can be updated in this regard as well and the inv messages removed?

@glozow
Copy link
Member Author

glozow commented May 3, 2023

Yes, PKGINFO is not specified anywhere; think of it as an "abstract class." The purpose of that diagram is to show how this protocol might be extended or upgraded in the future, i.e. we can define a new type of PKGINFO message to be used in invs and/or getdatas, and continue using the same getpkgtxns and pkgtxns message for transaction download. I want to separate those 2 concepts: exchanging package information and exchanging transaction data.

For example, CHUNKPKGINFO could contain a chunk (with cluster-based mempool), perhaps specifying the chunk fee and size as well. Originally, I proposed a PCKGINFO1 type which was a child and its parents to communicate a fee-bumping relationship.

The reason there is an "inv(PKGINFO)" there is to communicate that, while inv(ANCPKGINFO) is not allowed, some other kind of inv(PKGINFO) can be allowed in the future for a sender-initiated protocol.

I will update the diagram to make this more clear, thanks for the feedback and sorry for the confusion!

bip-0331.mediawiki Outdated Show resolved Hide resolved
suggests adding new p2p messages enabling nodes to request and share package-validation-related
information with one another, resulting in a more efficient and reliable way to propagate packages.

(2) Eliminate the need for txid-based transaction relay and make orphan handling more robust.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You still "need" txid-based relay for backwards compatibility.

Arguably, txid-based relay would be more efficient than the proposed protocol: if you see an orphan Z, whose parent Y does not spend any in-mempool txs, then this protocol would make three round trips:

  • GETDATA wtx(Z); TZ Z
  • GETDATA andpkginfo(Z); ANCPKGINFO Z, Y
  • GETPKGTXNS wtx(Y); PKGTXNS Y

but you could alternatively do two round trips:

  • GETDATA wtx(Z); TZ Z
  • GETDATA andpkginfo(Z), tx(Y); ANCPKGINFO Z, Y; TX Y

That approach would not reduce round-trips when receiving orphans that do have an in-mempool grandparent that the receiver does not already know about (but it wouldn't increase the number of roundtrips either), and would also require storing both the child and parents while waiting for any grandparents.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose it's true that this protocol is not going to have fewer round trips in every possible case of orphans, but we wouldn't have a way of detecting those cases on the receiver end.

bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Show resolved Hide resolved
bip-0331.mediawiki Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
@glozow
Copy link
Member Author

glozow commented Jun 14, 2023

Updated for suggestions from @ajtowns, thanks!

Major changes (still need to update implementation to reflect these):

Copy link

@ariard ariard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the note on maybe asymmetry around “ancpkginfo"


# The "ancpkginfo" message has the structure defined above, with pchCommand == "ancpkginfo".

# The "txns" field should contain a list of wtxids which constitute the ancestor package of the last wtxid. The sender should - but is not required to - sort these wtxids in topological order. The topological sort can be achieved by sorting the transactions by mempool acceptance order (if parents are always accepted before children). Nodes should not disconnect or punish a peer who provides a list not sorted in topological order.<ref>'''Why not include feerate information to help the receiver decide whether these transactions are worth downloading?'''
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side-note, I think the feerate cannot be trusted under BIP118 ANYPREVOUTANYSCRIPT, where the spent amount are not committed in the digest and transaction child A can be attached to parent X or parent Z of different amounts (of course there is the issue to re-bind the ANYPREVOUTANYSCRIPT on a parent by scriptpubkey only…). So I think this avoid to introduce a dependency in light of potential future consensus changes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Useful extra context, though I didn't add any of this to the text.

bip-0331.mediawiki Show resolved Hide resolved
bip-0331.mediawiki Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Show resolved Hide resolved
bip-0331.mediawiki Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
@glozow
Copy link
Member Author

glozow commented Jul 4, 2023

I think I've addressed all comments now.

Copy link
Member

@instagibbs instagibbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

nits only

bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
Copy link
Member Author

@glozow glozow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed the text-related comments

bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Show resolved Hide resolved
@glozow
Copy link
Member Author

glozow commented Aug 7, 2023

Changed combined hash to be single instead of double sha256

@instagibbs
Copy link
Member

re-ACK 02ec218

suggested textual additions and change to single-sha2 for combined hash

@glozow
Copy link
Member Author

glozow commented Jan 17, 2024

@luke-jr @kallewoof is anything else needed here?

@glozow
Copy link
Member Author

glozow commented Apr 11, 2024

@luke-jr pinging again, can we get this merged?

Copy link
Member

@sr-gi sr-gi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of comments/nits I found when going over this

bip-0331.mediawiki Outdated Show resolved Hide resolved
bip-0331.mediawiki Show resolved Hide resolved
bip-0331.mediawiki Outdated Show resolved Hide resolved
@glozow
Copy link
Member Author

glozow commented Apr 24, 2024

pinging again, if any BIP editors are available to have a look?

@jonatack jonatack merged commit e43c18b into bitcoin:master Apr 24, 2024
3 checks passed
@jonatack
Copy link
Member

ACK 632f143

@glozow glozow deleted the 2022-10-package-relay branch May 22, 2024 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.