Add ability for node to reject tx from mempool by number of tx inputs #2342

bitcartel · 2017-05-04T18:35:40Z

Implement short-term solution described in #2343 so that users can respond promptly to critical short-term problems caused by quadratic validation scaling, such as the getblocktemplate latency, block propagation latency, and mempool size inflation issues described in #2333.

daira

Some minor changes needed. Also needs a test.

daira · 2017-05-04T18:42:51Z

src/init.cpp

@@ -332,6 +332,7 @@ std::string HelpMessage(HelpMessageMode mode)
    strUsage += HelpMessageOpt("-dbcache=<n>", strprintf(_("Set database cache size in megabytes (%d to %d, default: %d)"), nMinDbCache, nMaxDbCache, nDefaultDbCache));
    strUsage += HelpMessageOpt("-loadblock=<file>", _("Imports blocks from external blk000??.dat file") + " " + _("on startup"));
    strUsage += HelpMessageOpt("-maxorphantx=<n>", strprintf(_("Keep at most <n> unconnectable transactions in memory (default: %u)"), DEFAULT_MAX_ORPHAN_TRANSACTIONS));
+    strUsage += HelpMessageOpt("-mempooltxvinlimit=<n>", _("If set, limit the number of inputs to a transaction which this node will accept into the mempool for relay and mining"));


I suggest "mempooltxinputlimit"; vin is internal jargon.

Also, it's the number of transparent inputs.

daira · 2017-05-04T18:59:54Z

src/init.cpp

+    if (mapArgs.count("-mempooltxvinlimit")) {
+        unsigned int limit = GetArg("-mempooltxvinlimit", UINT_MAX);
+        if (limit == 0) {
+            return InitError(_("Mempool transaction vin limit cannot be 0."));


Also change this if vin is dropped from the option name.

daira · 2017-05-04T19:01:11Z

src/init.cpp

@@ -1009,6 +1010,14 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
    }
 #endif

+    if (mapArgs.count("-mempooltxvinlimit")) {
+        unsigned int limit = GetArg("-mempooltxvinlimit", UINT_MAX);


I believe the default should be 40, but let's discuss that on #2343.

Default is now 0 which means no limit (code has changed in forced push)

daira · 2017-05-04T19:18:29Z

src/init.cpp

@@ -1009,6 +1010,14 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
    }
 #endif

+    if (mapArgs.count("-mempooltxvinlimit")) {
+        unsigned int limit = GetArg("-mempooltxvinlimit", UINT_MAX);
+        if (limit == 0) {


int64_t limit = GetArg("-mempooltxvinlimit", std::numeric_limits<int64_t>::max()); if (limit < 0) { return InitError(_("Mempool limit on transparent inputs to a transaction must be greater than 0.")); }

Have adopted language.

daira · 2017-05-04T19:27:03Z

src/main.cpp

+    // Node operator can choose to reject tx by number of inputs
+    if (mapArgs.count("-mempooltxvinlimit")) {
+        unsigned int n = tx.vin.size();
+        unsigned int limit = GetArg("-mempooltxvinlimit", UINT_MAX);


static_assert(std::numeric_limits<size_t>::max() >= std::numeric_limits<int64_t>::max(), "size_t too small"); size_t n = tx.vin.size(); size_t limit = (size_t) GetArg("-mempooltxvinlimit", std::numeric_limits<int64_t>::max());

zookozcash · 2017-05-05T00:46:41Z

Red flag: Nathan mistakenly thought that the Incident Response team had required this ticket to be prioritised for 1.0.9. In fact the opposite was true, the Incident Response team (me as Executive Coordinator) had decided that we would take steps to fix the problem in the short term explicitly under the assumption that this would not imply that this ticket would be prioritised for 1.0.9.

bitcartel · 2017-05-05T04:33:47Z

@daira Changes made. No test yet, is someone available to write that?

daira · 2017-05-05T06:04:15Z

src/init.cpp

@@ -332,6 +332,7 @@ std::string HelpMessage(HelpMessageMode mode)
    strUsage += HelpMessageOpt("-dbcache=<n>", strprintf(_("Set database cache size in megabytes (%d to %d, default: %d)"), nMinDbCache, nMaxDbCache, nDefaultDbCache));
    strUsage += HelpMessageOpt("-loadblock=<file>", _("Imports blocks from external blk000??.dat file") + " " + _("on startup"));
    strUsage += HelpMessageOpt("-maxorphantx=<n>", strprintf(_("Keep at most <n> unconnectable transactions in memory (default: %u)"), DEFAULT_MAX_ORPHAN_TRANSACTIONS));
+    strUsage += HelpMessageOpt("-mempooltxinputlimit=<n>", _("Set the maximum number of transparent inputs in a transaction which the mempool will accept (default: 0 = no limit applied)"));


Non-blocking: s/which/that/

daira

Other than the double-negative in the error message, ACK on the changes since my last review. Still needs a test.

daira · 2017-05-05T06:09:09Z

src/init.cpp

+    if (mapArgs.count("-mempooltxinputlimit")) {
+        int64_t limit = GetArg("-mempooltxinputlimit", 0);
+        if (limit < 0) {
+            return InitError(_("Mempool limit on transparent inputs to a transaction cannot not be negative"));


s/cannot not/cannot/

veikkoeeva · 2017-05-05T07:01:35Z

A kind proposal, if there would a comment in the code about the problems and the solution, it would be great. I have been following the discussion in chat and I think I'd agree that "limiting this is good", but necessarily the real reasonin and why some particular limit was chosen (even commenting in vein "this limit is an educated guess to quell most of the problems known at the time") and what would happen to the transactions if the check fails (transactions in limbo?).

daira · 2017-05-05T08:37:11Z

@veikkoeeva I'll answer that on #2343.

nathan-at-least · 2017-05-10T21:41:39Z

Concept NACK: #2343 (comment)

nathan-at-least · 2017-06-02T21:05:52Z

Considering for 1.0.10 release juxtaposed to related tickets.

str4d

utACK on the patch, but I agree with daira's string requests and that this needs a test.

str4d · 2017-06-14T12:41:44Z

I'm also +1 on merging this in 1.0.10. I agree with @daira that we need something that users can leverage now, while we work on the longer-term improvements. I also think that the release notes should be explicit about how this is a short-term fix, and that it will be replaced (not extended) in future (likely with some metric that takes into account the entire mempool, not just individual transactions).

based on the number of transparent inputs.

bitcartel · 2017-06-17T04:04:40Z

src/gtest/test_mempool.cpp

+    CValidationState state3;
+    CTransaction tx3(mtx);
+    EXPECT_FALSE(AcceptToMemoryPool(pool, state3, tx3, false, &missingInputs));
+    EXPECT_NE(state3.GetRejectReason(), "bad-txns-version-too-low");


Why are all the error messages in the test related to 'bad-txns-version-too-low' ? I only see it fired here:

bool CheckTransactionWithoutProofVerification(const CTransaction& tx, CValidationState &state ) { // Basic checks that don't depend on any context // Check transaction version if (tx.nVersion < MIN_TX_VERSION) { return state.DoS(100, error("CheckTransaction(): version too low"), REJECT_INVALID, "bad-txns-version-too-low"); }

Correct. I intentionally set tx.nVersion = 0 to trigger this error, as it's the first check that occurs after the -mempooltxinputlimit check, and it means that I didn't have to mock out a heap of global state. Think of that error as "the transaction passes the tx input limit check". The actual check itself doesn't return a reason, which is why I check that the reject reason is not bad-txns-version-too-low; we could possibly make things more explicit by checking instead that it is empty.

Could you copy the above explanation into the test? I take it we don't want to return state.DoS which updates global CValidationState state in init.cpp, rather than what we do right now which is simply return false (like some of the other code paths in AcceptToMemoryPool).

mempool rejection.

str4d

NACK

str4d · 2017-06-20T02:56:38Z

src/wallet/asyncrpcoperation_sendmany.cpp

+    if (isfromtaddr_) {
+        size_t limit = (size_t)GetArg("-mempooltxinputlimit", 0);
+        if (limit > 0) {
+            size_t n = t_inputs_.size();


At this point in the function, this is the list of all available inputs, not selected inputs; z_sendmany therefore fails incorrectly. This check needs to be shifted to just below where t_inputs_ is re-used for storing the selected inputs. We also need to re-write this code (likely as part of #1360) to prevent this kind of bug.

str4d

ACK

Also reduce number of z_sendmany calls made so test runs quicker.

bitcartel · 2017-06-20T22:53:52Z

@str4d can you check latest commit; fixed test and reduced time to run.
@daira can you review the tests which were added to the PR since you last commented?

daira · 2017-06-21T08:14:10Z

I'll review now.

daira

The comment about the order of checking tx version and number of inputs needs a response, otherwise utACK.

daira · 2017-06-21T08:27:26Z

src/gtest/test_mempool.cpp

+    // Create an obviously-invalid transaction
+    // We intentionally set tx.nVersion = 0 to reliably trigger an error, as
+    // it's the first check that occurs after the -mempooltxinputlimit check,
+    // and it means that we don't have to mock out a lot of global state.


Wait, input limiting occurs before the tx version check? How can we parse the transaction if it has an unknown version? The spec explicitly says you're not supposed to try to parse fields other than nVersion if the version is unrecognised. (This could lead to misreporting a transaction as being invalid for having too many inputs, when in fact that field at that position has changed its meaning entirely and the transaction should instead have been rejected for having an unknown version. Not critical, but it could lead to confusing log messages.)

I did wonder this myself. However:

The version check is solely that tx.nVersion >= MIN_TX_VERSION, where MIN_TX_VERSION = 1, so any positive integer is already accepted. This is in fact one way in which transaction replay prevention has been proposed upstream: make the version negative.

Transaction parsing already happens before AcceptToMemoryPool is called, so the spec should still be satisfied:

bool static ProcessMessage(...) { ... else if (strCommand == "tx") { vector<uint256> vWorkQueue; vector<uint256> vEraseQueue; CTransaction tx; vRecv >> tx; CInv inv(MSG_TX, tx.GetHash()); pfrom->AddInventoryKnown(inv); LOCK(cs_main); bool fMissingInputs = false; CValidationState state; pfrom->setAskFor.erase(inv.hash); mapAlreadyAskedFor.erase(inv); if (!AlreadyHave(inv) && AcceptToMemoryPool(mempool, state, tx, true, &fMissingInputs)) { ... } ... } ... } bool ProcessMessages(...) { ... bool fRet = false; try { fRet = ProcessMessage(pfrom, strCommand, vRecv, msg.nTime); boost::this_thread::interruption_point(); } catch (const std::ios_base::failure& e) { ... } ... }

A bystander note here, feel free to ignore, but could this reasoning be added as a comment to the code? I think it would greatly help others too and later when one is perhaps considering refactoring the code.

Thanks for the reminder @veikkoeeva - I've updated the first post in the conversation above, so when this is merged, it will also be in the commit message for the merge. I think anybody in the future who wants to refactor the code around mempooltxinputlimit will have the information on hand to do so.

I don't see this in the first post.

@daira You don't see the text beginning "Implement short-term... " ? Also the PR number should be part of the zkbot commit message, so a developer can find their way to this conversation.

daira · 2017-06-21T08:34:35Z

Actually I'll file another ticket about the parsing-of-unknown-tx-versions issue. That is not going to affect consensus so it can be addressed separately and does not need to block this PR.

str4d

Needs one more adjustment, which I will make now.

str4d · 2017-06-21T21:29:10Z

qa/rpc-tests/mempool_tx_input_limit.py

+        assert_equal(set(self.nodes[1].getrawmempool()), set())
+
+        # Should use two UTXOs and succeed
+        spend_taddr_id2 = self.nodes[1].sendtoaddress(node0_taddr, spend_taddr_output + spend_taddr_output - Decimal('1'))


The first call was changed from sendtoaddress to sendfrom; this one should be too.

str4d · 2017-06-21T21:30:30Z

qa/rpc-tests/mempool_tx_input_limit.py

-        self.call_z_sendmany(node0_zaddr, node1_taddr, spend_taddr_amount - spend_taddr_output - spend_taddr_output - Decimal('0.0001')) # note amount less fees
+        recipients.append({"address":self.nodes[1].getnewaddress(), "amount": spend_taddr_output})
+        recipients.append({"address":self.nodes[1].getnewaddress(), "amount": spend_taddr_output})
+        recipients.append({"address":self.nodes[1].getnewaddress(), "amount": spend_taddr_amount - spend_taddr_output - spend_taddr_output})


ACK; the first variant was left over from an earlier version where node 1 already had a balance, so I was trying to get everything into one t-addr, but couldn't make multiple TXOs in the same z_sendmany.

str4d

ACK

str4d · 2017-06-21T21:41:19Z

@zkbot r+

zkbot · 2017-06-21T21:41:20Z

📌 Commit 6ea58d1 has been approved by str4d

zkbot · 2017-06-21T21:41:24Z

⌛ Testing commit 6ea58d1 with merge 59de56e...

Add ability for node to reject tx from mempool by number of tx inputs Implement short-term solution described in #2343 so that users can respond promptly to critical short-term problems caused by quadratic validation scaling, such as the getblocktemplate latency, block propagation latency, and mempool size inflation issues described in #2333.

zkbot · 2017-06-21T22:39:55Z

☀️ Test successful - pr-merge
Approved by: str4d
Pushing 59de56e to master...

daira mentioned this pull request May 4, 2017

Add a config option to limit the number of transparent inputs for txns accepted to the mempool #2343

Closed

daira requested changes May 4, 2017

View reviewed changes

nathan-at-least added this to In Progress in Security and Stability May 4, 2017

bitcartel force-pushed the 1081__mempoolpatch branch from 607ef82 to 9c2a621 Compare May 5, 2017 04:25

daira reviewed May 5, 2017

View reviewed changes

daira requested changes May 5, 2017

View reviewed changes

daira requested a review from str4d May 5, 2017 06:14

nathan-at-least self-requested a review May 10, 2017 21:41

nathan-at-least moved this from In Progress to Blocked in Security and Stability May 11, 2017

nathan-at-least moved this from Blocked to Discussion in Security and Stability May 31, 2017

nathan-at-least mentioned this pull request May 31, 2017

Rejected Transactions Cache #2414

Closed

nathan-at-least added this to the 1.0.10 Release milestone Jun 2, 2017

jackgavigan mentioned this pull request Jun 12, 2017

Change transparent input selection to avoid excessive number of inputs #2411

Open

str4d requested changes Jun 14, 2017

View reviewed changes

Add option 'mempooltxinputlimit' so the mempool can reject a transaction

da6d939

based on the number of transparent inputs.

bitcartel force-pushed the 1081__mempoolpatch branch from 9c2a621 to da6d939 Compare June 17, 2017 02:32

Add test for -mempooltxinputlimit

5799c5f

bitcartel commented Jun 17, 2017

View reviewed changes

Check mempooltxinputlimit when creating a transaction to avoid local

9e84b5a

mempool rejection.

str4d requested changes Jun 20, 2017

View reviewed changes

str4d and others added 3 commits June 20, 2017 15:58

Additional testing of -mempooltxinputlimit

4ef0141

Partial revert & fix for commit 9e84b5a ; code block in wrong location.

d8616d0

Fix comment

b1eb4f2

str4d approved these changes Jun 20, 2017

View reviewed changes

Fix #b1eb4f2 so test checks sendfrom as originally intended.

99f6d5d

Also reduce number of z_sendmany calls made so test runs quicker.

daira approved these changes Jun 21, 2017

View reviewed changes

str4d requested changes Jun 21, 2017

View reviewed changes

Use sendfrom for both t-addr calls

6ea58d1

str4d approved these changes Jun 21, 2017

View reviewed changes

zkbot merged commit 6ea58d1 into zcash:master Jun 21, 2017

This was referenced Jun 22, 2017

pyflakes cleanup #2458

Merged

High transaction volumes causing problems for miners #2333

Closed

str4d moved this from Discussion to Complete in Security and Stability Jun 26, 2017

This was referenced Jun 28, 2017

Improve "immediate / local transaction failure mode" reporting #2492

Closed

Active Note / UTXO merging. #2493

Closed

bitcartel mentioned this pull request Jul 12, 2017

Add an RPC function or flag to create a shielding transaction that only contains coinbase #2448

Closed

WaveringAna mentioned this pull request Nov 18, 2017

Development HorizenOfficial/zen_archived#90

Merged

str4d mentioned this pull request Dec 2, 2017

Adapt SIGHASH_FORKID for (1) NU0 replay protection (2) fixing quadratic hashing (3) hardware wallets #2584

Closed

nathan-at-least mentioned this pull request Dec 20, 2017

Interim solution to "too many txins" problem #2818

Closed

Add ability for node to reject tx from mempool by number of tx inputs #2342

Add ability for node to reject tx from mempool by number of tx inputs #2342

Conversation

bitcartel commented May 4, 2017 • edited Loading

daira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zookozcash commented May 5, 2017

bitcartel commented May 5, 2017

daira May 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veikkoeeva commented May 5, 2017 • edited Loading

daira commented May 5, 2017

nathan-at-least commented May 10, 2017

nathan-at-least commented Jun 2, 2017

str4d left a comment

Choose a reason for hiding this comment

str4d commented Jun 14, 2017

bitcartel Jun 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

str4d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

str4d left a comment

Choose a reason for hiding this comment

bitcartel commented Jun 20, 2017

daira commented Jun 21, 2017

daira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daira Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daira commented Jun 21, 2017

str4d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

str4d left a comment

Choose a reason for hiding this comment

str4d commented Jun 21, 2017

zkbot commented Jun 21, 2017

zkbot commented Jun 21, 2017

zkbot commented Jun 21, 2017

bitcartel commented May 4, 2017 •

edited

Loading

daira May 5, 2017 •

edited

Loading

veikkoeeva commented May 5, 2017 •

edited

Loading

bitcartel Jun 17, 2017 •

edited

Loading

daira Jun 21, 2017 •

edited

Loading