Allow expedited blocks only from expedited nodes #486

ptschip · 2017-04-28T16:05:44Z

No description provided.

kyuupichan · 2017-04-30T02:24:45Z

src/parallel.cpp

@@ -553,13 +553,16 @@ void HandleBlockMessageThread(CNode *pfrom, const string &strCommand, const CBlo
        int nTotalThinBlocksInFlight = 0;
        {
            LOCK(cs_vNodes);
+            // Erase this thinblock from the tracking map now that we're done with it.
+            if (pfrom->mapThinBlocksInFlight.erase(inv.hash))


Does this need the map lock?

i'll update that...thanks.

kyuupichan · 2017-04-30T02:33:32Z

src/expedited.cpp

+            LOCK(cs_main);
+            Misbehaving(pfrom->GetId(), 100);
+            return error("Invalid EXPEDITED_MSG_XTHIN received");
+        }


I would feel much better if the validity checks for an xthin block and expedited block were the same code. Expedited is missing much of it. One is just an unrequested form of the other, so apart from minor details their handling should be identical.

kyuupichan · 2017-04-30T02:44:29Z

src/main.cpp

+            LOCK(cs_main);
+            Misbehaving(pfrom->GetId(), 100);
+            return error("XPEDITEDREQUEST message received from a non thinblock node, peer=%d", pfrom->GetId());
+        }


I continue to believe we're banning without good cause. This is easily detected and not harmful, a low misbehaving seems more appropriate in this and several other cases. Can we come up with a policy about what deserves a ban and apply it consistently?

I thought about the idea of such a policy, but didn't come up with any good solution except to move this policy into the hands of the node operator in the form of tweakable config:

https://bitco.in/forum/threads/buip053-tweakable-client-dos-responses.2074/

let's be more tolerant here. How about Misbehaving 1 just to stop lots of repeat requests. Also let's keep all the logic in HandleExpeditedRequest(). I think that this message handling function is too large and should be refactored to look simply like:
if (strCommand == NetMsgType::MSG1) HandleMSG1()
else if (strCommand == NetMsgType::MSG2) HandleMSG2()
...

and this HandleExpeditedRequest() is starting to go there. Make it return a boolean if we need an early return from the message handling.

sickpig · 2017-04-30T22:07:36Z

src/test/exploit_tests.cpp

-    ProcessMessage(&dummyNode1, NetMsgType::BUVERACK, vRecv1, GetTime());
-    SendMessages(&dummyNode1);
-    BOOST_CHECK(!dummyNode1.fBUVersionSent);
-    BOOST_CHECK(CNode::IsBanned(addr1));


the removal of this test has already being merged in via #490, right?

yes, i'll rebase a squash this out.

ptschip · 2017-05-04T15:08:53Z

@gandrewstone just fixed the merge conflict from the SENDHEADERS PR455

gandrewstone · 2017-05-04T15:11:12Z

src/expedited.cpp

+            return true;
+    }
+    return false;
+}


expedited partners can be set via RPC call and we'll want an automatic option in the future. So to discover whether a node is expedited, please use one of these global vars.
std::vector<CNode*> xpeditedBlk; // (256,(CNode*)NULL); // Who requested expedited blocks from us
std::vector<CNode*> xpeditedBlkUp; //(256,(CNode*)NULL); // Who we requested expedited blocks from
std::vector<CNode*> xpeditedTxn; // (256,(CNode*)NULL);

thanks i'll update...that's much better.

gandrewstone · 2017-05-04T15:22:32Z

src/main.cpp

+            return error("XPEDITEDBLK message received from a non thinblock node, peer=%d", pfrom->GetId());
+        }
+
+        // ignore the expedited message unless we are near the chain tip...
        if (!fImporting && !fReindex && IsChainNearlySyncd())
        {


similarly, let's put all this checking logic in HandleExpeditedBlock()

gandrewstone · 2017-05-04T15:26:24Z

src/main.cpp

@@ -5921,23 +5927,41 @@ bool ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vRecv, in
    }


-    else if (strCommand == NetMsgType::XPEDITEDREQUEST)
+    else if (strCommand == NetMsgType::XPEDITEDREQUEST && IsThinBlocksEnabled())


If conditions like (&& IsThinBlocksEnabled()) are added to these top level if statements, then the code will fall through to the final else that issues this log:
// Ignore unknown commands for extensibility
LogPrint("net", "Unknown command "%s" from peer=%d\n", SanitizeString(strCommand), pfrom->id);

I don't think we want that log printed if !IsThinBlocksEnabled(). Let's put the IsThinBlocksEnabled() in HandleExpeditedRequest()

gandrewstone · 2017-05-04T15:27:08Z

src/main.cpp

-    else if (strCommand == NetMsgType::XPEDITEDBLK)
+
+
+    else if (strCommand == NetMsgType::XPEDITEDBLK && IsThinBlocksEnabled() && IsExpeditedNode(pfrom))


As noted earlier, if additional conditions like (&& IsThinBlocksEnabled()) are added to these top level if statements, then the code will fall through to the final else that issues an "Unknown command" log.

If the remote peer or your peer do not have thinblocks enabled then do not process expedited messages. Add Thinblock message consistency checking for expedited transactions Do not process an expedited block from a peer you have not specified as such Do not allow urequested blocks/xthins/thinblocks unless from an expedited node

…the node This helps in two ways, to disconnect slow nodes but more importantly to clear out the thinblock data being held in memory for that node. Add locking for mapThinBlocksInFlight Check mapThinBlocksInflight as well as mapBlocksInFlight before banning There is a rare event that can happen when you have expedited processing turned on an by chance you re-request a full Block. If the expedited block beats your re-request then mapBlocksInFlight will be empty and you will end up banning a good node when the full Block returns from the re-request. This problem is a caused by mapBlocksInFlight not tracking by node id as well as hash. Lock mapThinBlocksInFlight before erasing elements, in parallel.cpp

Move checking into HandleExpeditedBlock and HandleExpeditedRequest Move IsThinBlocksEnabled() IsThinBlockCapable() and IsExpeditedNode() checks from main.cpp into the expedited functions.

… nodes, and increase logging during qa node cache creation and during validateblocktemplate test

ptschip · 2017-05-05T11:57:59Z

@gandrewstone merging your pull but still sendheaders.py is broken...I'm going to have to take out the bans on unrequested blocks, even though they are slight ones. There is no risk IMO, it's mainly a nuisance that some nodes send us unrequested blocks. The problem here is that mininode has issues with not being able to send blocks unrequested and since it will likely take some time to figure out a good approach to testing I think the best thing is to remove these misbehaviors for the time being.

Assiging misbehavior here is causing issues with some tests such as sendheaders.py that use mininode. We'll have to think about how best to proceed which doesn't break our tests. So taking this out for the time being.

1e6f1f5ad Merge BitcoinUnlimited#529: fix tests.c in the count == 0 case 95e99f196 fix tests.c in the count == 0 case 452d8e4d2 Merge BitcoinUnlimited#523: scratch: add stack frame support 6fe50439a scratch: add stack frame support 9bc2e2650 Merge BitcoinUnlimited#522: parameterize ecmult_const over input size 7c1b91ba4 parameterize ecmult_const over input size dbc3ddd5e Merge BitcoinUnlimited#513: Increase sparsity of pippenger fixed window naf representation fb9271dcf Merge BitcoinUnlimited#510: add a couple missing `const`s to ecmult_pippenger_wnaf cd5f6028e Merge BitcoinUnlimited#515: Fix typo 09146ae85 Merge BitcoinUnlimited#512: secp256k1_ec_privkey_negate - fix documentation ec0a7b3ae Don't touch leading zeros in wnaf_fixed. 9e36d1bfe Fix bug in wnaf_fixed where the wnaf array is not completely zeroed when given a 0 scalar. 96f68a0af Don't invert scalar in wnaf_fixed when it is even because a caller might intentionally give a scalar with many leading zeros. 9b7c47a21 Fix typo 6dbb00786 Increase sparsity of pippenger fixed window naf representation 1646ace4d secp256k1_ec_privkey_negate - fix documentation 9b3ff0309 add a couple missing `const`s to ecmult_pippenger_wnaf cd329dbc3 Merge BitcoinUnlimited#460: [build] Update ax_jni_include_dir.m4 macro 7f9c1a156 Merge BitcoinUnlimited#498: tests: Avoid calling fclose(...) with an invalid argument f99aa8d4d Merge BitcoinUnlimited#499: tests: Make sure we get the requested number of bytes from /dev/urandom b549d3d5f Merge BitcoinUnlimited#472: [build] Set --enable-jni to no by default instead of auto. d33352151 Merge BitcoinUnlimited#494: Support OpenSSL versions >= 1.1 for ENABLE_OPENSSL_TESTS 2ef8ea5d2 Merge BitcoinUnlimited#495: Add bench_ecmult to .gitignore 82a96e458 tests: Make sure we get the requested number of bytes from /dev/urandom 5aae5b5bb Avoid calling fclose(...) with an invalid argument cb32940df Add bench_ecmult to .gitignore 31abd3ab8 Support OpenSSL versions >= 1.1 for ENABLE_OPENSSL_TESTS c95f6f136 Merge BitcoinUnlimited#487: fix tests typo, s/changed/unchanged fb46c8388 Merge BitcoinUnlimited#463: Reduce usage of hardcoded size constants 02f5001df Merge BitcoinUnlimited#490: Disambiguate bench functions and types 1f46d6089 Disambiguate bench functions and types f54c6c508 Merge BitcoinUnlimited#480: Enable benchmark building by default c77fc0859 Merge BitcoinUnlimited#486: Add pippenger_wnaf for multi-multiplication d2f9c6b5d Use more precise pippenger bucket windows 4c950bbea Save some additions per window in _pippenger_wnaf a58f543f5 Add flags for choosing algorithm in ecmult_multi benchmark 36b22c933 Use scratch space dependent batching in ecmult_multi 355a38f11 Add pippenger_wnaf ecmult_multi bc65aa794 Add bench_ecmult dba5471b6 Add ecmult_multi tests 8c1c831bd Generalize Strauss to support multiple points 548de42ec add resizeable scratch space API 0e96cdc6b fix typo, s/changed/unchanged c7680e570 Reduce usage of hardcoded size constants 6ad5cdb42 Merge BitcoinUnlimited#479: Get rid of reserved _t in type names 7a78f6059 Print whether we're building benchmarks 4afec9f1a Build benchmarks by default d1dc9dfc0 Get rid of reserved _t in type names 0b7024185 Merge BitcoinUnlimited#474: Fix header guards using reserved identifiers ab1f89f00 Merge BitcoinUnlimited#478: Fixed multiple typos 8c7ea22d5 Fixed multiple typos abe2d3e84 Fix header guards using reserved identifiers 57752d28b [build] Set --enable-jni to no by default instead of auto. f532bdc9f Merge BitcoinUnlimited#459: Add pubkey prefix constants to include/secp256k1.h cac7c5559 Merge BitcoinUnlimited#470: Fix wnaf_const documentation 768514bac Fix wnaf_const documentation with respect to return value and number of words set b8c26a399 Merge BitcoinUnlimited#458: Fix typo in API documentation 817fb2013 Merge BitcoinUnlimited#440: Fix typos 12230f90e Merge BitcoinUnlimited#468: Remove redundant conditional expression 2e1ccdca0 Remove redundant conditional expression e7daa9b3c [build] Tweak JNI macro to warn instead of error for JNI not found. 5b2297792 [build] Update ax_jni_include_dir.m4 macro to deal with recent versions of macOS bc61b91ac add pubkey prefix constants to include/secp256k1.h b0452e664 Fix typo in API documentation 84973d393 Merge BitcoinUnlimited#454: Remove residual parts from the schnorr expirement. 5e95bf228 Remove residual parts from the schnorr expirement. cbc20b8c3 Merge BitcoinUnlimited#452: Minor optimizations to _scalar_inverse to save 4M 4cc8f5250 Merge BitcoinUnlimited#437: Unroll secp256k1_fe_(get|set)_b32 to make them much faster. 465159c27 Further shorten the addition chain for scalar inversion. a2b6b1914 Fix benchmark print_number infinite loop. 8b7680a82 Unroll secp256k1_fe_(get|set)_b32 for 10x26. aa8499080 Unroll secp256k1_fe_(get|set)_b32 for 5x52. cf12fa13c Minor optimizations to _scalar_inverse to save 4M 119949232 Merge BitcoinUnlimited#408: Add `secp256k1_ec_pubkey_negate` and `secp256k1_ec_privkey_negate` 6af087107 Merge BitcoinUnlimited#441: secp256k1_context_randomize: document. ab31a524b Merge BitcoinUnlimited#444: test: Use checked_alloc eda5c1a06 Merge BitcoinUnlimited#449: Remove executable bit from secp256k1.c 51b77ae61 Remove executable bit from secp256k1.c 5eb030ca4 test: Use checked_alloc 72d952c9c FIXUP: Missing "is" 70ff29b6a secp256k1_context_randomize: document. 4c0f32ed5 Fix typo: "Agressive" → "Aggressive" 73aca8364 Fix typo: "exectured" → "executed" 8e48aa60d Add `secp256k1_ec_pubkey_negate` and `secp256k1_ec_privkey_negate` git-subtree-dir: src/secp256k1 git-subtree-split: 1e6f1f5ad5e7f1e3ef79313ec02023902bf8175c

ptschip force-pushed the dev_expedited2 branch 5 times, most recently from 67c1d84 to 73d9f83 Compare April 29, 2017 16:56

kyuupichan reviewed Apr 30, 2017

View reviewed changes

sickpig added the needs backport label Apr 30, 2017

sickpig reviewed Apr 30, 2017

View reviewed changes

ptschip force-pushed the dev_expedited2 branch 2 times, most recently from 63b8770 to b27e80f Compare May 4, 2017 15:08

gandrewstone reviewed May 4, 2017

View reviewed changes

ptschip and others added 4 commits May 5, 2017 04:50

Use xpeditedBlkUp to determine whether we have an expdited node or not

0b81b67

Move checking into HandleExpeditedBlock and HandleExpeditedRequest Move IsThinBlocksEnabled() IsThinBlockCapable() and IsExpeditedNode() checks from main.cpp into the expedited functions.

solve block not requested race conditions, be more tolerant of slower…

2a04076

… nodes, and increase logging during qa node cache creation and during validateblocktemplate test

ptschip force-pushed the dev_expedited2 branch from 5239a52 to d310d6c Compare May 5, 2017 11:57

Take out the assigning of Misbehavior when un-requested block arrive

378c4eb

Assiging misbehavior here is causing issues with some tests such as sendheaders.py that use mininode. We'll have to think about how best to proceed which doesn't break our tests. So taking this out for the time being.

ptschip force-pushed the dev_expedited2 branch from d310d6c to 378c4eb Compare May 5, 2017 12:57

gandrewstone merged commit ac62a14 into BitcoinUnlimited:dev May 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow expedited blocks only from expedited nodes #486

Allow expedited blocks only from expedited nodes #486

ptschip commented Apr 28, 2017

kyuupichan Apr 30, 2017

ptschip May 1, 2017

kyuupichan Apr 30, 2017

kyuupichan Apr 30, 2017

ftrader May 3, 2017

gandrewstone May 4, 2017 •

edited

sickpig Apr 30, 2017

ptschip May 1, 2017

ptschip commented May 4, 2017

gandrewstone May 4, 2017

ptschip May 4, 2017

gandrewstone May 4, 2017

gandrewstone May 4, 2017

gandrewstone May 4, 2017 •

edited

ptschip commented May 5, 2017

		else if (strCommand == NetMsgType::XPEDITEDBLK)


		else if (strCommand == NetMsgType::XPEDITEDBLK && IsThinBlocksEnabled() && IsExpeditedNode(pfrom))

Allow expedited blocks only from expedited nodes #486

Allow expedited blocks only from expedited nodes #486

Conversation

ptschip commented Apr 28, 2017

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

gandrewstone May 4, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ptschip commented May 4, 2017

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

gandrewstone May 4, 2017 • edited

Choose a reason for hiding this comment

ptschip commented May 5, 2017

gandrewstone May 4, 2017 •

edited

gandrewstone May 4, 2017 •

edited