Make non-standard tx acceptance a peer option. #27772
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee 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. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
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. |
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. |
| @@ -71,15 +71,15 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t | |||
| if (max_tx_fee > 0) { | |||
| // First, call ATMP with test_accept and check the fee. If ATMP | |||
| // fails here, return error immediately. | |||
| const MempoolAcceptResult result = node.chainman->ProcessTransaction(tx, /*test_accept=*/ true); | |||
| const MempoolAcceptResult result = node.chainman->ProcessTransaction(tx, /*require_standard=*/false, /*test_accept=*/ true); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Are you suggesting to add -acceptnonstdtxnrpc?
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. |
Make
-acceptnonstdtxnapply to peers only and always allow local rpc calls to submit non-standard transactions to the mempool.Fixes #27768