Make non-standard tx acceptance a peer option.

joostjager commented at 11:55 am on May 28, 2023: none

Make -acceptnonstdtxn apply to peers only and always allow local rpc calls to submit non-standard transactions to the mempool.

Fixes https://github.com/bitcoin/bitcoin/issues/27768

Make non-standard tx acceptance a peer option. 4126cef8d9

DrahtBot commented at 11:55 am on May 28, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	sdaftuar

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:

#27742 ([NO MERGE] BIP331 Ancestor Package Relay by glozow)
#27675 (net_processing: Drop m_recently_announced_invs bloom filter by ajtowns)
#27499 (net processing, refactor: Decouple PeerManager from gArgs by dergoegge)
#26711 (validate package transactions with their in-package ancestor sets by glozow)
#26525 (Remove -mempoolfullrbf option by BitcoinErrorLog)

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.

DrahtBot added the label CI failed on May 28, 2023

sdaftuar commented at 7:07 pm on May 28, 2023: member

Rather than open more PRs that are duplicative of other recently rejected PRs, it would be more helpful to directly address the feedback that was previously given.

Are you unaware that some of the DoS attacks that are prevented by our standardness checks include situations where a block might be mined that would take a very long time to validate, a cost that would be felt by the whole network? See for instance https://bitcointalk.org/?topic=140078%7CCVE-2013-2292]]. It is absurd that this issue would be ignored in a PR seeking to change mainnet behavior.

Even putting aside the network-wide risks, I think it would be inappropriate for this project to bless a configuration that could lead to our users allowing themselves to be DoS’ed, which seems like the obvious outcome (I have no idea how we would provide documentation to our users to do these risk assessments themselves).

In the most recent PR that tried to do something similar to this (which you also commented on), I asked for the particular rules that seem to be problematic (for whatever use case is motivating these PRs) to be spelled out, so that we can discuss – and perhaps there are indeed modifications to our rules that would be fine and beneficial to the network if we adopted. If you’d care to do that here, perhaps progress could be made. Treating our standardness rules as a black box to be worked around, without any consideration for why any of these rules exist, is not helpful and not going to lead anywhere. Concept NACK.

joostjager commented at 12:17 pm on May 29, 2023: none

Rather than open more PRs that are duplicative of other recently rejected PRs, it would be more helpful to directly address the feedback that was previously given.

This PR was intended as a draft. The goal was not to propose any changes right away, but rather to illustrate (also for myself) the touch points in the codebase. I wouldn’t consider this PR a duplicate though. It only relaxes tx acceptance on the rpc level, and leaves the p2p restrictions in place - contrary to #27578. Nevertheless apologies if it came across as pushy.

I will reply to your other feedback points within the context of #27768, to avoid spreading out the discussion across multiple places.

in src/node/transaction.cpp:74 in 4126cef8d9

70@@ -71,15 +71,15 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t
71             if (max_tx_fee > 0) {
72                 // First, call ATMP with test_accept and check the fee. If ATMP
73                 // fails here, return error immediately.
74-                const MempoolAcceptResult result = node.chainman->ProcessTransaction(tx, /*test_accept=*/ true);
75+                const MempoolAcceptResult result = node.chainman->ProcessTransaction(tx, /*require_standard=*/false,  /*test_accept=*/ true);

maflcko commented at 1:16 pm on May 29, 2023:

Looks like you just switch this from true to false here, without making it an option. NACK on making this a silent footgun for all users.

joostjager commented at 1:22 pm on May 29, 2023:

Are you suggesting to add -acceptnonstdtxnrpc?

fanquake commented at 1:42 pm on May 29, 2023: member

I will reply to your other feedback points within the context of #27768, to avoid spreading out the discussion across multiple places.

I think it’d be better to close this PR for now, (I agree with suhas’s comments above), and continue discussion in #27768 (I see you’ve responded there with further details of what you are interested in).

I don’t think there’s a need to keep this PR open to discuss code changes at this point, and it can always be reopened in future. Given it was also intended to be an example of what changes you had in mind/would be required, you could link to it from #27768, either in the op, or in further commentary, as a POC.

joostjager closed this on May 29, 2023

luke-jr commented at 1:25 am on June 24, 2023: member

Make non-standard tx acceptance a peer option. #27772

Reviews

Conflicts