rpc: allow submitpackage to be called outside of regtest

glozow commented at 11:29 pm on May 9, 2023: member

Permit (restricted topology) submitpackage RPC outside of regtest. Suggested in #26933 (comment)

This RPC should be safe but still experimental - interface may change, not all features (e.g. package RBF) are implemented, etc. If a miner wants to expose this to people, they can effectively use “package relay” before the p2p changes are implemented. However, please note this is not package relay; transactions submitted this way will not relay to other nodes if the feerates are below their mempool min fee. Users should put this behind some kind of rate limit or permissions.

glozow added the label RPC/REST/ZMQ on May 9, 2023

DrahtBot commented at 11:29 pm on May 9, 2023: contributor

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.

Type	Reviewers
ACK	instagibbs, dergoegge, ajtowns, ariard, achow101
Concept ACK	jamesob, fanquake, TheBlueMatt
Approach ACK	darosior, murchandamus

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.

jamesob commented at 11:35 pm on May 9, 2023: member

Concept ACK

in src/rpc/mempool.cpp:765 in ec025db843 outdated

763+        "Submit a package of raw transactions (serialized, hex-encoded) to local node.\n"
764         "The package will be validated according to consensus and mempool policy rules. If all transactions pass, they will be accepted to mempool.\n"
765         "This RPC is experimental and the interface may be unstable. Refer to doc/policy/packages.md for documentation on package policies.\n"
766-        "Warning: until package relay is in use, successful submission does not mean the transaction will propagate to other nodes on the network.\n"
767-        "Currently, each transaction is broadcasted individually after submission, which means they must meet other nodes' feerate requirements alone.\n"
768+        "Warning: unless this node and others are using package relay (-packagerelay), successful submission does not mean the transactions will propagate throughout the network.\n"

instagibbs commented at 2:34 pm on May 10, 2023:

the string packagerelay doesn’t exist yet in the codebase

glozow commented at 3:43 pm on May 10, 2023:

Fixed

instagibbs commented at 2:34 pm on May 10, 2023: member

concept ACK

glozow force-pushed on May 10, 2023

glozow commented at 3:44 pm on May 10, 2023: member

Added a release note and addressed #27609 (review)

fanquake commented at 5:12 pm on May 10, 2023: member

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.

glozow force-pushed on May 10, 2023

glozow commented at 7:27 pm on May 10, 2023: member

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:

 0     grandparent
 1       5650sat
 2        5649vB
 3     /    |    \
 4  parent1 |   parent2
 513223sat  |   13223sat
 6  191vB   |   191vB
 7       \  |   /
 8        child
 9       2424sat
10        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.

instagibbs commented at 7:43 pm on May 10, 2023: member

note that the :gem: 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

glozow commented at 8:01 pm on May 10, 2023: member

note that the 💎 issue still exists in #26711

Yeah. Rebasing it on top of this.

glozow requested review from sdaftuar on May 10, 2023

instagibbs commented at 1:24 pm on May 11, 2023: member

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

in src/test/txpackage_tests.cpp:404 in 7786861dcc outdated

399+        std::vector<COutPoint> grandparent_inputs;
400+        for (auto i{1}; i < 50; ++i) {
401+            grandparent_input_txns.push_back(m_coinbase_txns[i]);
402+            grandparent_inputs.push_back(COutPoint{m_coinbase_txns[i]->GetHash(), 0});
403+        }
404+        CAmount last_value = 49*50*COIN - 10*COIN - 10*COIN - grandparent_fee;

instagibbs commented at 3:28 pm on May 11, 2023:

grandparent_inputs.size() * COIN

glozow commented at 5:44 pm on May 11, 2023:

Done in #26711

in src/test/txpackage_tests.cpp:382 in 7786861dcc outdated

377+        // Diamond shape:
378+        //
379+        //     grandparent
380+        //       5650sat
381+        //        5649vB
382+        //     /         \

instagibbs commented at 3:32 pm on May 11, 2023:

test/txpackage_tests.cpp:382:9: warning: multi-line comment [-Wcomment] 382 | // /
| ^

glozow commented at 5:44 pm on May 11, 2023:

Replaced with carats https://github.com/bitcoin/bitcoin/compare/0cdb25a2392328afaad7651a0a407396b1922f17..f4e449254e41172b09a41ed9b2cc5c5fe3a5edca

in src/test/txpackage_tests.cpp:376 in 7786861dcc outdated

369@@ -368,6 +370,94 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup)
370         BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_parent->GetHash())));
371         BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_child->GetHash())));
372     }
373+
374+    // do not allow parents to pay for children
375+    {
376+        Package package_ppfc;

instagibbs commented at 3:56 pm on May 11, 2023:

Lots of magic numbers in this test. Could you assert specific vsizes on each tx, and have that number match the picture here?

glozow commented at 5:45 pm on May 11, 2023:

Done in #26711 (https://github.com/bitcoin/bitcoin/compare/0cdb25a2392328afaad7651a0a407396b1922f17..f4e449254e41172b09a41ed9b2cc5c5fe3a5edca)

glozow commented at 5:36 pm on May 11, 2023: member

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!

glozow force-pushed on May 11, 2023

glozow closed this on May 11, 2023

instagibbs commented at 5:46 pm on May 11, 2023: member

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

glozow commented at 5:54 pm on May 11, 2023: member

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.

joostjager commented at 9:17 am on May 12, 2023: none

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.

glozow commented at 6:42 pm on May 12, 2023: member

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.

instagibbs commented at 7:35 pm on May 12, 2023: member

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 ?

jamesob commented at 7:43 pm on May 12, 2023: member

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.

glozow commented at 8:17 pm on May 12, 2023: member

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.

glozow removed review request from sdaftuar on May 12, 2023

joostjager commented at 9:09 am on May 13, 2023: none

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.

joostjager commented at 9:12 am on May 13, 2023: none

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.

instagibbs commented at 4:30 pm on May 15, 2023: member

@glozow ok so the tree thing is a bandaid in lieu of #26711 , and you’d rather just to that @joostjager

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?

Also can have weird effects like your mempool min rate going higher than it should normally.

I think we can easily get #26711 in next release

joostjager commented at 4:44 pm on May 15, 2023: none

Also can have weird effects like your mempool min rate going higher than it should normally.

Is this something that #26711 prevents? Because it seems to me that that PR is less restrictive than the tree-only allowance in this PR.

instagibbs commented at 4:47 pm on May 15, 2023: member

To be clear, the tree structure in this PR “should” also stop it.

joostjager commented at 4:50 pm on May 15, 2023: none

By the way, about the diamond example in #27609 (comment)

This isn’t a “child with parents” package and also wouldn’t be accepted on regtest with the current master branch because the child has no direct link to the grandparent? (https://github.com/bitcoin/bitcoin/blob/d7700d3a26478d9b1648463c188648c7047b1c60/src/policy/packages.cpp#L68)

glozow commented at 4:55 pm on May 15, 2023: member

By the way, about the diamond example in #27609 (comment)

This isn’t a “child with parents” package

Sorry, that was a bad diagram (of the unit test I had added). The child is also spending the grandparent. I’ve updated the comment now.

joostjager commented at 5:01 pm on May 15, 2023: none

It is still not completely clear to me what the risk is with merging this PR if #27611 is less restrictive. It would be great if it makes the next release, but also it is another six months delay for this important step.

instagibbs commented at 5:17 pm on May 15, 2023: member

If we’re not shooting for a backport for 25.x, I don’t think the time difference is meaningful? This PR is adding stuff that will directly be removed and is sort of a distraction for reviewers themselves.

(maybe backport was the idea :P )

joostjager commented at 5:54 pm on May 15, 2023: none

Yes, backport was the idea 😎 😃

instagibbs commented at 6:12 pm on May 15, 2023: member

fwiw I’m ok getting this in for backport. This is most cruft at the RPC layer that can be reverted on the next PR.

edit: basically:

get this merged
backport
get 26711 in which removes the cruft

sdaftuar commented at 8:12 pm on May 15, 2023: member

Just want to point out that our project’s philosophy has generally been to only backport bugfixes, not features – and IMO it’s hard to see this as a bugfix.

joostjager commented at 8:17 pm on May 15, 2023: none

There is a bugfix in here, because I think it fixes the submitpackage acceptance policy on regtest by restricting it to a safe subset of topologies? Users may use regtest now to prepare for usage of the rpc on mainnet later.

glozow commented at 2:59 pm on May 16, 2023: member

This wouldn’t be backported. The first 2 commits could be considered a band aid “fix” but for a regtest-only RPC. The last 2 commits contain a new feature for miners, and we do not backport new features.

I understand this is a big limitation for Lightning, and I agree it should be treated with high priority. The way to fix it is to provide a way to submit and propagate packages to miners. If you would like to make package relay available in Bitcoin Core, please review #26711.

joostjager commented at 3:14 pm on May 16, 2023: none

Even just backporting the first two commits could be helpful, because it at least reduces the size of the patch that a miner needs to run to safely accept packages.

ajtowns commented at 3:52 am on June 12, 2023: contributor

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 this should have remained as a separate PR, and should have been okay to merge as-is, rather than adding only loosely-related commits to #26711. Two reasons:

after #26933 this doesn’t allow any problematic behaviour that would not have also been achievable without this patchset, simply by having received the txs in a different order (eg, by sending you the individual txs in the package immediately when your node restarted, after it had connected to p2p but before it had finished loading the mempool)
it’s an experimental rpc; if people using it hit problems occasionally, that’s okay.

Yes, there are definitely edge cases and possibly problematic behaviour, but there’s already lots of that in the mempool, and there’s nothing here that’s particularly scary.

That said, I don’t think we should be encouraging miners to expose this prior to the rest of the package relay work; and given #26933 this doesn’t really provide any benefits over just having a large enough mempool that you’re not evicting transactions in the first place, and running getblocktemplate against that. (Or just using prioritise transaction on the ancestors to bump them up to an acceptable fee rate)

joostjager commented at 10:00 am on June 15, 2023: none

this doesn’t really provide any benefits over just having a large enough mempool that you’re not evicting transactions in the first place, and running getblocktemplate against that

There may be miners/mining pools that have a more elaborate setup with multiple nodes. This PR could allow miners to accept packages into a standard size mempool of a peripheral node, which peers with the main node that has a much larger mempool and against which gbt is run. This allows them to isolate out-of-band package acceptance from their main operation.

For a impression of how the submitpackage rpc could for example be used on a mempool instance: https://github.com/mempool/mempool/pull/3859.

From the ongoing discussion it seems that #26711 isn’t completely trivial. If this PR #27609 is indeed viewed as not allowing any problematic behaviour that would not have also been achievable without this patchset, could it be worth consideration for inclusion in 25.1? As mentioned elsewhere, the limited topology enabled by this PR already solves the most pressing lightning need.

glozow commented at 2:17 pm on September 28, 2023: member

Since the last time we discussed, we’ve fixed some bugs and added more testing (see #28471, #28472, #28251, #28450) so I think we made the right call closing at the time.

There seems to be sustained interest in having this. Still not a fan of encouraging direct submission to miners, but people have suggested that this is safer than the non-Bitcoin Core solutions that people are using / considering.

So reopening this. I think the main questions we should discuss are:

Is this safe enough to expose on mainnet (with documented warnings)?
Should we include the “IsTree” restriction or just open it as-is?

glozow reopened this on Sep 28, 2023

glozow commented at 2:17 pm on September 28, 2023: member

cc @instagibbs @TheBlueMatt @darosior @ajtowns @sdaftuar

instagibbs commented at 2:19 pm on September 28, 2023: member

concept ACK

I’m fine with revising this as a MVP for Bitcoin Core 26.0(feature freeze Oct 8) to get people experimenting/using in limited ways.

At this point I’m also fine removing the tree constraint, letting it be up to the caller to use topologies they’re comfortable with.

#26711 work will continue to remove additional incentive-compatibility footguns.

DrahtBot added the label Needs rebase on Sep 28, 2023

fanquake added this to the milestone 26.0 on Sep 29, 2023

ajtowns commented at 5:11 pm on September 29, 2023: contributor

There seems to be sustained interest in having this. Still not a fan of encouraging direct submission to miners, but people have suggested that this is safer than the non-Bitcoin Core solutions that people are using / considering.

I don’t think we should go out of our way to “encourage direct submission to miners”, but I also don’t think making the feature available counts as “encouraging”. I think the warning added to the RPC (“successful submission does not mean the transactions will propagate”) is sufficient.

So reopening this. I think the main questions we should discuss are:
* Should we include the "IsTree" restriction or just open it as-is?

I think it makes sense to have a “child + ancestors” restriction (as opposed to either submitting independent packages in a single call, or dealing with two-children combined pay for a common parent); I don’t mind restricting that to “child + direct parents”, though would prefer more general; 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. Those are just my preferences, though, Concept ACK for whichever.

instagibbs commented at 5:36 pm on September 29, 2023: member

I don’t mind restricting that to “child + direct parents”, though would prefer more general;

That’s what’s in master until #26711 to be clear. So the local choice here is as-is or more restrictive, and you’re against additional constraints.

glozow force-pushed on Oct 2, 2023

DrahtBot removed the label Needs rebase on Oct 2, 2023

[doc] parent pay for child in aggregate CheckFeeRate b4f28cc345

[txpackages] IsChildWithParentsTree()

Many edge cases exist when parents in a child-with-parents package can
spend each other. However, this pattern should also be uncommon in
normal use cases.

e32ba1599c

[rpc] require package to be a tree in submitpackage 5b9087a9a7

[rpc] allow submitpackage to be called outside of regtest 7a9bb2a2a5

[doc] add release note for submitpackage 5b878be742

glozow force-pushed on Oct 2, 2023

glozow commented at 9:23 am on October 2, 2023: member

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.

darosior commented at 11:54 am on October 2, 2023: member

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.)

TheBlueMatt commented at 4:01 pm on October 2, 2023: contributor

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.

instagibbs commented at 4:04 pm on October 2, 2023: member

ACK 5b878be742dbfcd232d949d2df1fff4743aec3d8

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.

achow101 commented at 8:15 pm on October 4, 2023: member

Concept ACK

in src/policy/packages.h:69 in e32ba1599c outdated

62@@ -63,4 +63,8 @@ bool CheckPackage(const Package& txns, PackageValidationState& state);
63  */
64 bool IsChildWithParents(const Package& package);
65 
66+/** Context-free check that a package IsChildWithParents() and none of the parents depend on each
67+ * other (the package is a "tree").
68+ */
69+bool IsChildWithParentsTree(const Package& package);

ajtowns commented at 4:15 am on October 5, 2023:

Seems like this would be better as:

0class Package {
1public:
2    std::vector<CTransactionRef> txs;
3    bool IsChildWithParentsTree() const;
4    ...
5};

in doc/release-notes-27609.md:4 in 5b878be742

0@@ -0,0 +1,14 @@
1+- A new RPC, `submitpackage`, has been added. It can be used to submit a list of raw hex
2+  transactions to the mempool to be evaluated as a package using consensus and mempool policy rules.
3+These policies include package CPFP, allowing a child with high fees to bump a parent below the
4+mempool minimum feerate (but not minimum relay feerate).

ajtowns commented at 4:17 am on October 5, 2023:

“current mempool minimum feerate” might make it a bit clearer that this is the dynamic level, not the fixed 1sat/vb level?

glozow commented at 7:44 am on October 5, 2023: member

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.

dergoegge approved

dergoegge commented at 10:29 am on October 5, 2023: member

Code review ACK 5b878be742dbfcd232d949d2df1fff4743aec3d8

Code and docs look good to me. Didn’t do any testing.

murchandamus commented at 12:03 pm on October 5, 2023: contributor

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.

ajtowns commented at 2:11 pm on October 5, 2023: contributor

ACK 5b878be742dbfcd232d949d2df1fff4743aec3d8

Did some basic tests and it worked as expected. Some nits noted that could be addressed in a followup.

darosior commented at 2:37 pm on October 5, 2023: member

@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.

instagibbs commented at 3:15 pm on October 5, 2023: member

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.

ajtowns commented at 6:31 pm on October 5, 2023: contributor

As I understand it:

what we’ve got now (in master) is simple – all the complex cases are in #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)

in doc/release-notes-27609.md:14 in 5b878be742

 9+    - Not all features are available. The package is limited to a child with all of its
10+      unconfirmed parents, and no parent may spend the output of another parent.  Also, package
11+      RBF is not supported. Refer to doc/policy/packages.md for more details on package policies
12+      and limitations.
13+
14+    - This RPC is experimental. Its interface may change.

ariard commented at 7:07 pm on October 5, 2023:

“policies and limitations” can change if sounds more robust and consistent with future p2p support i think

in src/policy/packages.cpp:98 in 5b878be742

93+{
94+    if (!IsChildWithParents(package)) return false;
95+    std::unordered_set<uint256, SaltedTxidHasher> parent_txids;
96+    std::transform(package.cbegin(), package.cend() - 1, std::inserter(parent_txids, parent_txids.end()),
97+                   [](const auto& ptx) { return ptx->GetHash(); });
98+    // Each parent must not have an input who is one of the other parents.

ariard commented at 7:12 pm on October 5, 2023:

from my understanding a child input conflicting with a parent input is checked later on in ProcessNewPackage / AcceptPackage / `CheckPackage

in src/rpc/mempool.cpp:826 in 5b878be742

824+        "The package must consist of a child with its parents, and none of the parents may depend on one another.\n"
825         "The package will be validated according to consensus and mempool policy rules. If all transactions pass, they will be accepted to mempool.\n"
826         "This RPC is experimental and the interface may be unstable. Refer to doc/policy/packages.md for documentation on package policies.\n"
827-        "Warning: until package relay is in use, successful submission does not mean the transaction will propagate to other nodes on the network.\n"
828-        "Currently, each transaction is broadcasted individually after submission, which means they must meet other nodes' feerate requirements alone.\n"
829+        "Warning: successful submission does not mean the transactions will propagate throughout the network.\n"

ariard commented at 7:13 pm on October 5, 2023:

could say the integrality of the transactions, to convey the idea than transactions might propagate partially e.g only the parents not child

ariard approved

ariard commented at 7:27 pm on October 5, 2023: member

Code Review ACK 5b878be742. Though didn’t manually test the PR.

I think you have few high-level concerns if submitpackage is exposed by mining pools or block templates builders (e.g transaction “accelerators”) or other substantial transaction-relay full-nodes :

it can be maliciously leveraged to broadcast a high-fee, low-feerate pinning package of LN commitment transaction + child still above the dynamic mempool min fee blocking a honest high-fee, high-feerate single commitment tx to propagate and confirm
it can be used to throw high-fee low feerate packages in your competitors mining pools to block single high ancestor score individual transaction propagating and as such downgrade their total income
it can be used by p2p deanonymization attackers to fingerprint network mempools and observe altered transaction-relay propagation to discover tx-relay topology (I guess similar to the old TxProbe strategy)

If correct, I think all those concerns are more transient “swallowing the bullet” style until full p2p packages is deployed. On the other hand, it’s good to have early testing in limited production of the package acceptance logic, so I would say it’s an equilibrium.

I don’t think if those concerns are correct they warrants withholding landing this PR, though I believe releases notes or doc/policy/packages.md could be extended accordingly as follow-up to say what it is relevant in function of each submitpackage deployment.

achow101 commented at 11:02 pm on October 5, 2023: member

ACK 5b878be742dbfcd232d949d2df1fff4743aec3d8

achow101 merged this on Oct 5, 2023

achow101 closed this on Oct 5, 2023

glozow deleted the branch on Oct 8, 2023

rpc: allow submitpackage to be called outside of regtest #27609

Code Coverage

Reviews

Conflicts