The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #26711 (validate package transactions with their in-package ancestor sets by glozow)
• #25038 (policy: nVersion=3 and Package RBF by glozow)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Concept ACK

Added a release note and addressed #27609 (comment)

Concept ACK - when you get a chance, can you put together a backport PR for 25.x, so we can see the changes. I think if we want to do that, it'd be preferable to get it into rc2.

Had a chat with @sdaftuar offline about how safe this RPC is for a miner to expose. He gave an example of a "parent pays for child" package that is currently handled poorly, and a small fix to prevent it. I've added the small fix and a test that contains an example.

Imagine a diamond shape and mempool minfeerate is 5sat/vB:

     grandparent
       5650sat
        5649vB
     /    |    \
  parent1 |   parent2
13223sat  |   13223sat
  191vB   |   191vB
       \  |   /
        child
       2424sat
        485vB

On master, what we'll do is reject each individual transaction, but end up accepting the full package. The child is just below the mempool min feerate and is likely to be evicted later in TrimToSize(), but doing so pushes the min feerate up by 1sat/vB and leaves the grandparent + parents in the mempool. Each of the parents' ancestor packages have low block selection scores but the grandparent's descendant package looks pretty good. Maybe one day we'll eliminate the asymmetry between selection and eviction... but basically our problem here is allowing the parents to pay for the child in the first place.

You can perhaps submit the same transactions via sendrawtransaction if -maxmempool is very large and the min feerate is lower. However, the fact that this pushes mempool min feerate up makes this worse than what you can do without it - the mempool will start rejecting anything between 5-6sat/vB afterwards.

The small change is to reject any package in which the child has a lower feerate than the package feerate. So we reject all of the transactions.

note that the 💎 issue still exists in #26711 , where the ultimate subpackage(including the child) would let everything in.

with the fix, hopefully, now every sub-package that is let in the mempool must have a child that pays

note that the 💎 issue still exists in #26711

Yeah. Rebasing it on top of this.

without #26711 we are still letting things into mempool that are possibly problematic, no?

without #26711 we are still letting things into mempool that are possibly problematic, no?

You're right. Every time we think about it a bit longer, another not-quite-ideal edge case comes up. Especially with full mempools and eviction being something to consider.

I think it was too early to open this. I've pushed a change to disallow any dependency between parents, which is where all these stupid edge cases come from (but is probably not a common use case).

Concept ACK - when you get a chance, can you put together a backport PR for 25.x, so we can see the changes.

With the "tree only" topology requirement I feel more comfortable having this branch exist, but still don't think this is something to merge yet / backport. If anybody saw this PR and got excited they could maybe run this, but I'm going to close this PR.

@instagibbs thanks for your comments, I'm incorporating them into #26711!

IsChildWithParentsTree <---- is this originally what the V3 topo looked like?

IsChildWithParentsTree <---- is this originally what the V3 topo looked like?

Yep exactly. 1 child multiple parents. Parents can't spend each other. You can use this for batch-bumping commitment transactions, for example.

With the "tree only" topology requirement I feel more comfortable having this branch exist, but still don't think this is something to merge yet / backport.

Do you think this is not ready to merge / backport yet because of the unexpected edge cases that showed up with the previous, less restrictive iteration, leading to the expectation that something might show up for "tree only" too? Or is there already a known issue for "tree only"?

I am looking at this from a Lightning perspective. Not being able to get that presigned commitment transaction and htlc sweep in before cltv expiry is bad and something that may not be so difficult to exploit for an attacker. Of course this PR only allows acceptance of a package over rpc, but at least it would allow miners to open up an alternative endpoint for vulnerable lightning node operators to submit their packages to.

With the "tree only" topology requirement I feel more comfortable having this branch exist, but still don't think this is something to merge yet / backport.

Do you think this is not ready to merge / backport yet because of the unexpected edge cases that showed up with the previous, less restrictive iteration, leading to the expectation that something might show up for "tree only" too? Or is there already a known issue for "tree only"?

I am looking at this from a Lightning perspective. Not being able to get that presigned commitment transaction and htlc sweep in before cltv expiry is bad and something that may not be so difficult to exploit for an attacker. Of course this PR only allows acceptance of a package over rpc, but at least it would allow miners to open up an alternative endpoint for vulnerable lightning node operators to submit their packages to.

Short answer: I don't know of any issues with the "child-with-parents tree only" topology. It was actually made specifically for Lightning.

The current RPC is restricted to child-with-parents topology. However, it doesn't work for all child-with-parent packages as it doesn't handle parents spending each other properly. This is one of the reasons it was regtest-only. This, among other things, was going to be fixed in #26711, after which it would be ok to build the p2p changes (and thus make sense to submitpackage outside of regtest).

Given current events, multiple people reached out about a miner opening up submitpackage to make "package relay without the relay" available for lightning today. So I made this PR, thinking it was "good enough" for this use case and RPC is a trusted interface anyway. But after talking through the edge cases more with others, particularly about a miner opening this as a public API, I became convinced that this is unsafe and we should wait until #26711. It didn't seem right to quickly add commits that avoided specific edge cases, as there are just so many of them. #26711 would not make sense to rush or backport.

But I already opened this PR, so I wanted to at least push a branch that I don't believe is unsafe. Hence the "tree only" which avoids the edge cases but covers Lightning since AFAIK you won't make commitment transactions that spend each other. I do not recommend running a patch, but if you decide to do so, use this branch with trees only.

re:topology restrictions beyond generalized ancestor packages, is the thought to restrict the typologies at a higher level, vs AcceptMultipleTransactions? Noticed this branch is restricting at RPC layer. I presume it would also require tree structure in #26711 ?

I became convinced that this is unsafe

I'm curious, what's the worst-case failure here? Is it that a participating miner's mempool gets screwed up? or that replacement may not happen properly? or that pinning becomes an issue?

In other words, I'm wondering if the failure is tolerable in the sense that the damage would be contained to the user trying to submit a dumb (or "not topoolgically sensical") package?

I was enthusiastic about a small change that would open up this functionality for emergency use to, say, facilitate closing a channel whose commitment tx no longer is admissable to a high-fee mempool, but things have gotten complicated and I'm now having a tough time following.

Still down to review, just wondering if theoretical edge cases are getting in the way of good functionality.

re:topology restrictions beyond generalized ancestor packages, is the thought to restrict the typologies at a higher level, vs AcceptMultipleTransactions? Noticed this branch is restricting at RPC layer. I presume it would also require tree structure in #26711 ?

No tree-only in #26711, the hope is to handle ancestor packages so we can do ancestor package relay. We can do it 💪
This restriction is RPC-level because it's basically a "batched commitment cpfp only" tack-on.

I'm curious, what's the worst-case failure here? Is it that a participating miner's mempool gets screwed up? or that replacement may not happen properly? or that pinning becomes an issue?

The worst case is the miner accepts transactions they actually should reject.

Yes, if you're using this for fee-bumping commitment transactions, you're not going to submit any of these edge-casey packages.

covers Lightning since AFAIK you won't make commitment transactions that spend each other

That's true. For anchor commitments I think it is the case that all outputs except the anchors (main balance outputs and htlcs) are timelocked CSV 1, so they aren't even available until after confirmation.

The worst case is the miner accepts transactions they actually should reject.

If I understood the various threads here and in other prs correctly, I think the consequence of this is that the mempool will be sub-optimal and lead to a block template that earns fewer fees for the miner exposing the endpoint?

If this would happen, I suppose the miner can always close the endpoint again and wait for the next release that is more strict on the "child-with-parents tree only" packages (while also allowing different topologies). The upside is that the edge case is identified at that point.

But also this is only a theoretical possibility, because it might be that there are no edges cases with "child-with-parents tree only"? Not sure if this is provable somehow. If there would be an edge case, I think it would need to be filtered out by #26711 (assuming that PR fixes it all). Maybe that is a way to come up with that edge case - try to find a "child-with-parents tree only" that would pass this PR, but not be able to pass the validation in #26711? When looking at #26711 though, it seems that with testing all sub-packages, it is only relaxing things rather than restricting it further compared to this PR.

I also wanted to share two other thoughts:

• Perhaps more unexpected problems emerge for the more generic "all child-with-parent packages" fix, further delaying the deployment of submitpackage for lightning use cases.

• I think that the availability of submitpackage outside of regtest is a trigger for lightning devs to start looking at it more seriously and consider the implications for node implementations. It may not be completely trivial to use the new rpc, because the nodes might need to keep track of packages internally and this may require refactors. By kicking off that process, it can continue in parallel with further ironing out the other topologies on L1 rather than being blocked on it.

  Same for miners. I think it is advantageous if their thinking about package acceptance is started sooner rather than later. These benefits may outweigh the slight uncertainties that still surround this PR.

Rebased.

At this point I'm also fine removing the tree constraint

I'd prefer to avoid the "IsChildWithParentsTree" restriction (ie, parents do not spend each other), but if it's necessary it would be good to add an explanation in the code comments of what edge cases we're concerned about precisely.

It doesn't seem like people are strongly against / have use cases hindered by the IsTree. I've kept it and added some comments about the edge cases in validation.cpp. I'm most concerned about the DoSyness of accepting a child paid for by its parents and then evicting it again later.

Concept ACK and Approach ACK (regarding the IsTree check).

We want to provide a safer interface for miners who may currently be reached-out to for accepting packages before package relay is available. We want to avoid introducing a footgun with this command, where using it could get a miner's mempool to refuse transactions it would otherwise accept (either because it was filled with suboptimal transactions which wouldn't get evicted or because the minfeerate was artificially bumped more than necessary).

There is a few arguments in this thread along the line of "this situation can already be reached by using multiple sendrawtransaction calls anyways". I don't think this should be the bar for avoiding footguns here though? I'm not saying we should bend over backward to make an RPC interface safe to use by untrusted parties but if users still need to roll out their own incentive compatibility checks this does not achieve the stated goal of being an alternative to their custom solutions.

Moreover it seems there is at the moment only interest in transmitting very specific packages out of band, Lightning commitment transaction + a fee-paying child and Lightning HTLC sweep transaction + a fee-paying child. Given recent bugs, the remaining uncertainty and the proximity to the release i think we should definitely keep the IsTree check. I would also be fine with limiting this to one child + one parent on mainnet as it seems to cover the only usecase that would make miners do custom things. (But i also can't think of something that would be prevented by this limitation that isn't already prevented by IsTree.)

Concept ACK having this exposed - in general even simple packages with a parent and a child where the parent has too low a fee to reach the mempool will propagate reasonably - many nodes don't actually ever hit their mempool min fee because all their peers do it for them, so they sit right below 300MB usage and don't ever go over and set a min fee. Some miners likely run unlimited mempools (or, if they don't, they can mine on a small mempool and use this rpc to submit packages from an unlimited mempool node sitting next to it). Thus, for those with lots of peers, there's some nontrivial probability of packages actually making it to miners even if they wouldn't accept it on P2P, thus exposing this is a quite useful even without the P2P parts.

ACK 5b878be

Nothing much changed from last time.

Tree topology will be useful for most people that will be submitting locally(one parent and one child often), and it's a quite trivial check that we can back out later. Did some very basic testing on mainnet.

Concept ACK

Will need a couple more ACKs to get this in for 26.0 - @fanquake @ajtowns @darosior @achow101 @joostjager @jamesob? The diff is very small, mostly docs.

Given that I understood right, that any package where the child has a lower feerate than the package feerate is rejected:

Concept ACK, Approach ACK, will start reviewing today.

@murchandamus

Given that I understood right, that any package where the child has a lower feerate than the package feerate is rejected

I don't think that's the case. You can submitpackage with a package that is not a CPFP.

I don't think that's the case. You can submitpackage with a package that is not a CPFP.

The child transaction should be rejected, but not the entire package.

As I understand it:

• what we've got now (in master) is simple -- all the complex cases are in validate package transactions with their in-package ancestor sets #26711. the approach here is just to fail if we hit a complex case, and not worry about it. after that PR is merged, then this RPC can support complex cases too, but not until then.
• the simple case is "we have a set of (unrelated) parents, and a single child that spends them"
• it's implemented as:
  • iterate over each parent, try to accept it -- if it's above minfee, it'll go straight in
  • try what remains as a package -- ie, a bunch of below minfee parents, and a child. if either the child or the package is below minfee, abort
• because we limit to unrelated parents, there's no chance that accepting one parent will allow some other parent to be accepted, etc

So I think this isn't quite accurate:

Given that I understood right, that any package where the child has a lower feerate than the package feerate is rejected:

The only way you get a child with a lower feerate than the package, is if a parent has a higher feerate than the child and the package, but in that case the parent will have already been accepted, and the package will become smaller, and this is repeated until the child does have the highest feerate in the package.

(Personally I'm not overly concerned about the risk of not having optimal txs (particularly at the bottom of the mempool) -- you can already get similar behaviour by (eg) having a public node with 300MB maxmempool and a private node with 500MB maxmempool that only receives txs from your public node (thus mostly ensuring its minfee stays at 0; perhaps add a short mempool expiry, too), then manually adding the txs in question to your mempool, and using your mempool to generate blocks, etc. It's an experimantal rpc, so if you don't like how it ends up working, that's fine; wait 'til it's not experimental before using it. Just my opinion, YMMV of course)

ACK 5b878be

fwiw, people who like this may also like #31385