From 80175472d1a9687da704c5180bda173596271b12 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 18 Dec 2016 23:03:16 -0800 Subject: [PATCH 01/14] Make CBlockIndex*es in net_processing const --- src/net_processing.cpp | 48 +++++++++++++++++++++--------------------- src/validation.cpp | 6 ++++-- src/validation.h | 2 +- 3 files changed, 29 insertions(+), 27 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index ccfbb77fc..4374b5be3 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -101,7 +101,7 @@ namespace { /** Blocks that are in flight, and that are in the queue to be downloaded. Protected by cs_main. */ struct QueuedBlock { uint256 hash; - CBlockIndex* pindex; //!< Optional. + const CBlockIndex* pindex; //!< Optional. bool fValidatedHeaders; //!< Whether this block has validated headers at the time of request. std::unique_ptr partialBlock; //!< Optional, used for CMPCTBLOCK downloads }; @@ -156,13 +156,13 @@ struct CNodeState { //! List of asynchronously-determined block rejections to notify this peer about. std::vector rejects; //! The best known block we know this peer has announced. - CBlockIndex *pindexBestKnownBlock; + const CBlockIndex *pindexBestKnownBlock; //! The hash of the last unknown block this peer has announced. uint256 hashLastUnknownBlock; //! The last full block we both have. - CBlockIndex *pindexLastCommonBlock; + const CBlockIndex *pindexLastCommonBlock; //! The best header we have sent our peer. - CBlockIndex *pindexBestHeaderSent; + const CBlockIndex *pindexBestHeaderSent; //! Length of current-streak of unconnecting headers announcements int nUnconnectingHeaders; //! Whether we've started headers synchronization with this peer. @@ -331,7 +331,7 @@ bool MarkBlockAsReceived(const uint256& hash) { // Requires cs_main. // returns false, still setting pit, if the block was already in flight from the same peer // pit will only be valid as long as the same cs_main lock is being held -bool MarkBlockAsInFlight(NodeId nodeid, const uint256& hash, const Consensus::Params& consensusParams, CBlockIndex *pindex = NULL, list::iterator **pit = NULL) { +bool MarkBlockAsInFlight(NodeId nodeid, const uint256& hash, const Consensus::Params& consensusParams, const CBlockIndex *pindex = NULL, list::iterator **pit = NULL) { CNodeState *state = State(nodeid); assert(state != NULL); @@ -432,7 +432,7 @@ bool CanDirectFetch(const Consensus::Params &consensusParams) } // Requires cs_main -bool PeerHasHeader(CNodeState *state, CBlockIndex *pindex) +bool PeerHasHeader(CNodeState *state, const CBlockIndex *pindex) { if (state->pindexBestKnownBlock && pindex == state->pindexBestKnownBlock->GetAncestor(pindex->nHeight)) return true; @@ -443,7 +443,7 @@ bool PeerHasHeader(CNodeState *state, CBlockIndex *pindex) /** Find the last common ancestor two blocks have. * Both pa and pb must be non-NULL. */ -CBlockIndex* LastCommonAncestor(CBlockIndex* pa, CBlockIndex* pb) { +const CBlockIndex* LastCommonAncestor(const CBlockIndex* pa, const CBlockIndex* pb) { if (pa->nHeight > pb->nHeight) { pa = pa->GetAncestor(pb->nHeight); } else if (pb->nHeight > pa->nHeight) { @@ -462,7 +462,7 @@ CBlockIndex* LastCommonAncestor(CBlockIndex* pa, CBlockIndex* pb) { /** Update pindexLastCommonBlock and add not-in-flight missing successors to vBlocks, until it has * at most count entries. */ -void FindNextBlocksToDownload(NodeId nodeid, unsigned int count, std::vector& vBlocks, NodeId& nodeStaller, const Consensus::Params& consensusParams) { +void FindNextBlocksToDownload(NodeId nodeid, unsigned int count, std::vector& vBlocks, NodeId& nodeStaller, const Consensus::Params& consensusParams) { if (count == 0) return; @@ -490,8 +490,8 @@ void FindNextBlocksToDownload(NodeId nodeid, unsigned int count, std::vectorpindexLastCommonBlock == state->pindexBestKnownBlock) return; - std::vector vToFetch; - CBlockIndex *pindexWalk = state->pindexLastCommonBlock; + std::vector vToFetch; + const CBlockIndex *pindexWalk = state->pindexLastCommonBlock; // Never fetch further than the best block we know the peer has, or more than BLOCK_DOWNLOAD_WINDOW + 1 beyond the last // linked block we have in common with this peer. The +1 is so we can detect stalling, namely if we would be able to // download that next block if the window were 1 larger. @@ -514,7 +514,7 @@ void FindNextBlocksToDownload(NodeId nodeid, unsigned int count, std::vectorIsValid(BLOCK_VALID_TREE)) { // We consider the chain that this peer is on invalid. return; @@ -1049,7 +1049,7 @@ void static ProcessGetData(CNode* pfrom, const Consensus::Params& consensusParam } } -uint32_t GetFetchFlags(CNode* pfrom, CBlockIndex* pprev, const Consensus::Params& chainparams) { +uint32_t GetFetchFlags(CNode* pfrom, const CBlockIndex* pprev, const Consensus::Params& chainparams) { uint32_t nFetchFlags = 0; if ((pfrom->GetLocalServices() & NODE_WITNESS) && State(pfrom->GetId())->fHaveWitness) { nFetchFlags |= MSG_WITNESS_FLAG; @@ -1456,7 +1456,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, LOCK(cs_main); // Find the last block the caller has in the main chain - CBlockIndex* pindex = FindForkInGlobalIndex(chainActive, locator); + const CBlockIndex* pindex = FindForkInGlobalIndex(chainActive, locator); // Send the rest of the chain if (pindex) @@ -1552,7 +1552,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, } CNodeState *nodestate = State(pfrom->GetId()); - CBlockIndex* pindex = NULL; + const CBlockIndex* pindex = NULL; if (locator.IsNull()) { // If locator is null, return the hashStop block @@ -1775,7 +1775,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, } } - CBlockIndex *pindex = NULL; + const CBlockIndex *pindex = NULL; CValidationState state; if (!ProcessNewBlockHeaders({cmpctblock.header}, state, chainparams, &pindex)) { int nDoS; @@ -2051,7 +2051,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, return true; } - CBlockIndex *pindexLast = NULL; + const CBlockIndex *pindexLast = NULL; { LOCK(cs_main); CNodeState *nodestate = State(pfrom->GetId()); @@ -2128,8 +2128,8 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, // If this set of headers is valid and ends in a block with at least as // much work as our tip, download as much as possible. if (fCanDirectFetch && pindexLast->IsValid(BLOCK_VALID_TREE) && chainActive.Tip()->nChainWork <= pindexLast->nChainWork) { - vector vToFetch; - CBlockIndex *pindexWalk = pindexLast; + vector vToFetch; + const CBlockIndex *pindexWalk = pindexLast; // Calculate all the blocks we'd need to switch to pindexLast, up to a limit. while (pindexWalk && !chainActive.Contains(pindexWalk) && vToFetch.size() <= MAX_BLOCKS_IN_TRANSIT_PER_PEER) { if (!(pindexWalk->nStatus & BLOCK_HAVE_DATA) && @@ -2151,7 +2151,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, } else { vector vGetData; // Download as much as possible, from earliest to latest. - BOOST_REVERSE_FOREACH(CBlockIndex *pindex, vToFetch) { + BOOST_REVERSE_FOREACH(const CBlockIndex *pindex, vToFetch) { if (nodestate->nBlocksInFlight >= MAX_BLOCKS_IN_TRANSIT_PER_PEER) { // Can't download any more from this peer break; @@ -2740,7 +2740,7 @@ bool SendMessages(CNode* pto, CConnman& connman, std::atomic& interruptMsg bool fRevertToInv = ((!state.fPreferHeaders && (!state.fPreferHeaderAndIDs || pto->vBlockHashesToAnnounce.size() > 1)) || pto->vBlockHashesToAnnounce.size() > MAX_BLOCKS_TO_ANNOUNCE); - CBlockIndex *pBestIndex = NULL; // last header queued for delivery + const CBlockIndex *pBestIndex = NULL; // last header queued for delivery ProcessBlockAvailability(pto->id); // ensure pindexBestKnownBlock is up-to-date if (!fRevertToInv) { @@ -2751,7 +2751,7 @@ bool SendMessages(CNode* pto, CConnman& connman, std::atomic& interruptMsg BOOST_FOREACH(const uint256 &hash, pto->vBlockHashesToAnnounce) { BlockMap::iterator mi = mapBlockIndex.find(hash); assert(mi != mapBlockIndex.end()); - CBlockIndex *pindex = mi->second; + const CBlockIndex *pindex = mi->second; if (chainActive[pindex->nHeight] != pindex) { // Bail out if we reorged away from this block fRevertToInv = true; @@ -2828,7 +2828,7 @@ bool SendMessages(CNode* pto, CConnman& connman, std::atomic& interruptMsg const uint256 &hashToAnnounce = pto->vBlockHashesToAnnounce.back(); BlockMap::iterator mi = mapBlockIndex.find(hashToAnnounce); assert(mi != mapBlockIndex.end()); - CBlockIndex *pindex = mi->second; + const CBlockIndex *pindex = mi->second; // Warn if we're announcing a block that is not on the main chain. // This should be very rare and could be optimized out. @@ -3013,10 +3013,10 @@ bool SendMessages(CNode* pto, CConnman& connman, std::atomic& interruptMsg // vector vGetData; if (!pto->fClient && (fFetch || !IsInitialBlockDownload()) && state.nBlocksInFlight < MAX_BLOCKS_IN_TRANSIT_PER_PEER) { - vector vToDownload; + vector vToDownload; NodeId staller = -1; FindNextBlocksToDownload(pto->GetId(), MAX_BLOCKS_IN_TRANSIT_PER_PEER - state.nBlocksInFlight, vToDownload, staller, consensusParams); - BOOST_FOREACH(CBlockIndex *pindex, vToDownload) { + BOOST_FOREACH(const CBlockIndex *pindex, vToDownload) { uint32_t nFetchFlags = GetFetchFlags(pto, pindex->pprev, consensusParams); vGetData.push_back(CInv(MSG_BLOCK | nFetchFlags, pindex->GetBlockHash())); MarkBlockAsInFlight(pto->GetId(), pindex->GetBlockHash(), consensusParams, pindex); diff --git a/src/validation.cpp b/src/validation.cpp index ca1e5a713..a6466334a 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3030,12 +3030,14 @@ static bool AcceptBlockHeader(const CBlockHeader& block, CValidationState& state } // Exposed wrapper for AcceptBlockHeader -bool ProcessNewBlockHeaders(const std::vector& headers, CValidationState& state, const CChainParams& chainparams, CBlockIndex** ppindex) +bool ProcessNewBlockHeaders(const std::vector& headers, CValidationState& state, const CChainParams& chainparams, const CBlockIndex** ppindex) { { LOCK(cs_main); for (const CBlockHeader& header : headers) { - if (!AcceptBlockHeader(header, state, chainparams, ppindex)) { + // cast away the ppindex-returns-const CBlockIndex - we're just assigning it to a CBlockIndex* + // that we own and is updated non-const anyway + if (!AcceptBlockHeader(header, state, chainparams, const_cast(ppindex))) { return false; } } diff --git a/src/validation.h b/src/validation.h index 981f65963..be49b0e65 100644 --- a/src/validation.h +++ b/src/validation.h @@ -243,7 +243,7 @@ bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr& block, CValidationState& state, const CChainParams& chainparams, CBlockIndex** ppindex=NULL); +bool ProcessNewBlockHeaders(const std::vector& block, CValidationState& state, const CChainParams& chainparams, const CBlockIndex** ppindex=NULL); /** Check whether enough disk space is available for an incoming block */ bool CheckDiskSpace(uint64_t nAdditionalBytes = 0); From 9a0b2f4c5b5cbdf37c68710ab8af877bd322d99c Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 4 Jan 2017 01:28:28 +0100 Subject: [PATCH 02/14] [qa] Make compact blocks test construction using fetch methods --- qa/rpc-tests/p2p-compactblocks.py | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/qa/rpc-tests/p2p-compactblocks.py b/qa/rpc-tests/p2p-compactblocks.py index fc1f16c6d..9151ecf5d 100755 --- a/qa/rpc-tests/p2p-compactblocks.py +++ b/qa/rpc-tests/p2p-compactblocks.py @@ -310,6 +310,9 @@ class CompactBlocksTest(BitcoinTestFramework): tip = int(node.getbestblockhash(), 16) assert(test_node.wait_for_block_announcement(tip)) + # Make sure we will receive a fast-announce compact block + self.request_cb_announcements(test_node, node, version) + # Now mine a block, and look at the resulting compact block. test_node.clear_block_announcement() block_hash = int(node.generate(1)[0], 16) @@ -319,27 +322,36 @@ class CompactBlocksTest(BitcoinTestFramework): [tx.calc_sha256() for tx in block.vtx] block.rehash() - # Don't care which type of announcement came back for this test; just - # request the compact block if we didn't get one yet. + # Wait until the block was announced (via compact blocks) wait_until(test_node.received_block_announcement, timeout=30) assert(test_node.received_block_announcement()) + # Now fetch and check the compact block + header_and_shortids = None + with mininode_lock: + assert(test_node.last_cmpctblock is not None) + # Convert the on-the-wire representation to absolute indexes + header_and_shortids = HeaderAndShortIDs(test_node.last_cmpctblock.header_and_shortids) + self.check_compactblock_construction_from_block(version, header_and_shortids, block_hash, block) + + # Now fetch the compact block using a normal non-announce getdata with mininode_lock: - if test_node.last_cmpctblock is None: - test_node.clear_block_announcement() - inv = CInv(4, block_hash) # 4 == "CompactBlock" - test_node.send_message(msg_getdata([inv])) + test_node.clear_block_announcement() + inv = CInv(4, block_hash) # 4 == "CompactBlock" + test_node.send_message(msg_getdata([inv])) wait_until(test_node.received_block_announcement, timeout=30) assert(test_node.received_block_announcement()) - # Now we should have the compactblock + # Now fetch and check the compact block header_and_shortids = None with mininode_lock: assert(test_node.last_cmpctblock is not None) # Convert the on-the-wire representation to absolute indexes header_and_shortids = HeaderAndShortIDs(test_node.last_cmpctblock.header_and_shortids) + self.check_compactblock_construction_from_block(version, header_and_shortids, block_hash, block) + def check_compactblock_construction_from_block(self, version, header_and_shortids, block_hash, block): # Check that we got the right block! header_and_shortids.header.calc_sha256() assert_equal(header_and_shortids.header.sha256, block_hash) From 8baaba653ef67097eb3d3a4b4d25f907111830b1 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 19 Dec 2016 00:47:08 -0800 Subject: [PATCH 03/14] [qa] Avoid race in preciousblock test. If node 0 is sufficiently fast to announce its block to node 1, node 1 might already have the block by the time the node_sync_via_rpc loop gets around to node 1, resulting in the submitblock result "duplicate-inconclusive" as node 1 has the block, but prefers an alternate chain. --- qa/rpc-tests/preciousblock.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qa/rpc-tests/preciousblock.py b/qa/rpc-tests/preciousblock.py index f43160e19..a80629464 100755 --- a/qa/rpc-tests/preciousblock.py +++ b/qa/rpc-tests/preciousblock.py @@ -102,7 +102,7 @@ class PreciousTest(BitcoinTestFramework): assert_equal(self.nodes[2].getblockcount(), 6) hashL = self.nodes[2].getbestblockhash() print("Connect nodes and check no reorg occurs") - node_sync_via_rpc(self.nodes[0:3]) + node_sync_via_rpc(self.nodes[1:3]) connect_nodes_bi(self.nodes,1,2) connect_nodes_bi(self.nodes,0,2) assert_equal(self.nodes[0].getbestblockhash(), hashH) From 180586fd44c3154af846e18850c83d0ac1296787 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 18 Dec 2016 23:24:11 -0800 Subject: [PATCH 04/14] Call AcceptBlock with the block's shared_ptr instead of CBlock& --- src/validation.cpp | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index a6466334a..d6bd78964 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3047,8 +3047,10 @@ bool ProcessNewBlockHeaders(const std::vector& headers, CValidatio } /** Store block on disk. If dbp is non-NULL, the file is known to already reside on disk */ -static bool AcceptBlock(const CBlock& block, CValidationState& state, const CChainParams& chainparams, CBlockIndex** ppindex, bool fRequested, const CDiskBlockPos* dbp, bool* fNewBlock) +static bool AcceptBlock(const std::shared_ptr& pblock, CValidationState& state, const CChainParams& chainparams, CBlockIndex** ppindex, bool fRequested, const CDiskBlockPos* dbp, bool* fNewBlock) { + const CBlock& block = *pblock; + if (fNewBlock) *fNewBlock = false; AssertLockHeld(cs_main); @@ -3128,7 +3130,7 @@ bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptrnPos = nBlockPos; blkdat.SetLimit(nBlockPos + nSize); blkdat.SetPos(nBlockPos); - CBlock block; + std::shared_ptr pblock = std::make_shared(); + CBlock& block = *pblock; blkdat >> block; nRewind = blkdat.GetPos(); @@ -3773,7 +3776,7 @@ bool LoadExternalBlockFile(const CChainParams& chainparams, FILE* fileIn, CDiskB if (mapBlockIndex.count(hash) == 0 || (mapBlockIndex[hash]->nStatus & BLOCK_HAVE_DATA) == 0) { LOCK(cs_main); CValidationState state; - if (AcceptBlock(block, state, chainparams, NULL, true, dbp, NULL)) + if (AcceptBlock(pblock, state, chainparams, NULL, true, dbp, NULL)) nLoaded++; if (state.IsError()) break; @@ -3800,16 +3803,17 @@ bool LoadExternalBlockFile(const CChainParams& chainparams, FILE* fileIn, CDiskB std::pair::iterator, std::multimap::iterator> range = mapBlocksUnknownParent.equal_range(head); while (range.first != range.second) { std::multimap::iterator it = range.first; - if (ReadBlockFromDisk(block, it->second, chainparams.GetConsensus())) + std::shared_ptr pblockrecursive = std::make_shared(); + if (ReadBlockFromDisk(*pblockrecursive, it->second, chainparams.GetConsensus())) { - LogPrint("reindex", "%s: Processing out of order child %s of %s\n", __func__, block.GetHash().ToString(), + LogPrint("reindex", "%s: Processing out of order child %s of %s\n", __func__, pblockrecursive->GetHash().ToString(), head.ToString()); LOCK(cs_main); CValidationState dummy; - if (AcceptBlock(block, dummy, chainparams, NULL, true, &it->second, NULL)) + if (AcceptBlock(pblockrecursive, dummy, chainparams, NULL, true, &it->second, NULL)) { nLoaded++; - queue.push_back(block.GetHash()); + queue.push_back(pblockrecursive->GetHash()); } } range.first++; From 69872195773870de8ee6521c9f555d60395a5ad9 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 18 Dec 2016 23:26:20 -0800 Subject: [PATCH 05/14] Add a CValidationInterface::NewPoWValidBlock callback --- src/validation.cpp | 5 +++++ src/validationinterface.cpp | 3 +++ src/validationinterface.h | 6 ++++++ 3 files changed, 14 insertions(+) diff --git a/src/validation.cpp b/src/validation.cpp index d6bd78964..20d75d957 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3096,6 +3096,11 @@ static bool AcceptBlock(const std::shared_ptr& pblock, CValidation return error("%s: %s", __func__, FormatStateMessage(state)); } + // Header is valid/has work, merkle tree and segwit merkle tree are good...RELAY NOW + // (but if it does not build on our best tip, let the SendMessages loop relay it) + if (!IsInitialBlockDownload() && chainActive.Tip() == pindex->pprev) + GetMainSignals().NewPoWValidBlock(pindex, pblock); + int nHeight = pindex->nHeight; // Write block to history file diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 215c342de..d4121a28b 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -22,6 +22,7 @@ void RegisterValidationInterface(CValidationInterface* pwalletIn) { g_signals.BlockChecked.connect(boost::bind(&CValidationInterface::BlockChecked, pwalletIn, _1, _2)); g_signals.ScriptForMining.connect(boost::bind(&CValidationInterface::GetScriptForMining, pwalletIn, _1)); g_signals.BlockFound.connect(boost::bind(&CValidationInterface::ResetRequestCount, pwalletIn, _1)); + g_signals.NewPoWValidBlock.connect(boost::bind(&CValidationInterface::NewPoWValidBlock, pwalletIn, _1, _2)); } void UnregisterValidationInterface(CValidationInterface* pwalletIn) { @@ -34,6 +35,7 @@ void UnregisterValidationInterface(CValidationInterface* pwalletIn) { g_signals.UpdatedTransaction.disconnect(boost::bind(&CValidationInterface::UpdatedTransaction, pwalletIn, _1)); g_signals.SyncTransaction.disconnect(boost::bind(&CValidationInterface::SyncTransaction, pwalletIn, _1, _2, _3)); g_signals.UpdatedBlockTip.disconnect(boost::bind(&CValidationInterface::UpdatedBlockTip, pwalletIn, _1, _2, _3)); + g_signals.NewPoWValidBlock.disconnect(boost::bind(&CValidationInterface::NewPoWValidBlock, pwalletIn, _1, _2)); } void UnregisterAllValidationInterfaces() { @@ -46,4 +48,5 @@ void UnregisterAllValidationInterfaces() { g_signals.UpdatedTransaction.disconnect_all_slots(); g_signals.SyncTransaction.disconnect_all_slots(); g_signals.UpdatedBlockTip.disconnect_all_slots(); + g_signals.NewPoWValidBlock.disconnect_all_slots(); } diff --git a/src/validationinterface.h b/src/validationinterface.h index 717026389..594072719 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -8,6 +8,7 @@ #include #include +#include class CBlock; class CBlockIndex; @@ -40,6 +41,7 @@ protected: virtual void BlockChecked(const CBlock&, const CValidationState&) {} virtual void GetScriptForMining(boost::shared_ptr&) {}; virtual void ResetRequestCount(const uint256 &hash) {}; + virtual void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& block) {}; friend void ::RegisterValidationInterface(CValidationInterface*); friend void ::UnregisterValidationInterface(CValidationInterface*); friend void ::UnregisterAllValidationInterfaces(); @@ -66,6 +68,10 @@ struct CMainSignals { boost::signals2::signal&)> ScriptForMining; /** Notifies listeners that a block has been successfully mined */ boost::signals2::signal BlockFound; + /** + * Notifies listeners that a block which builds directly on our current tip + * has been received and connected to the headers tree, though not validated yet */ + boost::signals2::signal&)> NewPoWValidBlock; }; CMainSignals& GetMainSignals(); From c80209214208621b84da10895427e1e5f381d1b8 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 5 Jan 2017 10:31:39 -0500 Subject: [PATCH 06/14] Relay compact block messages prior to full block connection --- src/net_processing.cpp | 33 +++++++++++++++++++++++++++++++++ src/net_processing.h | 1 + 2 files changed, 34 insertions(+) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 4374b5be3..15bdef86f 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -752,6 +752,39 @@ void PeerLogicValidation::SyncTransaction(const CTransaction& tx, const CBlockIn } } +void PeerLogicValidation::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& pblock) { + CBlockHeaderAndShortTxIDs cmpctblock(*pblock, true); + CNetMsgMaker msgMaker(PROTOCOL_VERSION); + + LOCK(cs_main); + + static int nHighestFastAnnounce = 0; + if (pindex->nHeight <= nHighestFastAnnounce) + return; + nHighestFastAnnounce = pindex->nHeight; + + bool fWitnessEnabled = IsWitnessEnabled(pindex->pprev, Params().GetConsensus()); + uint256 hashBlock(pblock->GetHash()); + + connman->ForEachNode([this, &cmpctblock, pindex, &msgMaker, fWitnessEnabled, &hashBlock](CNode* pnode) { + // TODO: Avoid the repeated-serialization here + if (pnode->nVersion < INVALID_CB_NO_BAN_VERSION || pnode->fDisconnect) + return; + ProcessBlockAvailability(pnode->GetId()); + CNodeState &state = *State(pnode->GetId()); + // If the peer has, or we announced to them the previous block already, + // but we don't think they have this one, go ahead and announce it + if (state.fPreferHeaderAndIDs && (!fWitnessEnabled || state.fWantsCmpctWitness) && + !PeerHasHeader(&state, pindex) && PeerHasHeader(&state, pindex->pprev)) { + + LogPrint("net", "%s sending header-and-ids %s to peer %d\n", "PeerLogicValidation::NewPoWValidBlock", + hashBlock.ToString(), pnode->id); + connman->PushMessage(pnode, msgMaker.Make(NetMsgType::CMPCTBLOCK, cmpctblock)); + state.pindexBestHeaderSent = pindex; + } + }); +} + void PeerLogicValidation::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) { const int nNewHeight = pindexNew->nHeight; connman->SetBestHeight(nNewHeight); diff --git a/src/net_processing.h b/src/net_processing.h index 230d805bd..9d0d79448 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -24,6 +24,7 @@ public: virtual void SyncTransaction(const CTransaction& tx, const CBlockIndex* pindex, int nPosInBlock); virtual void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload); virtual void BlockChecked(const CBlock& block, const CValidationState& state); + virtual void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& pblock); }; struct CNodeStateStats { From 9eaec08dd236a9e73c7993d25274ec913c999c63 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 14 Dec 2016 16:28:22 -0800 Subject: [PATCH 07/14] Cache most-recently-announced block's shared_ptr --- src/net_processing.cpp | 51 +++++++++++++++++++++++++++++++++--------- 1 file changed, 40 insertions(+), 11 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 15bdef86f..5de103d4e 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -752,6 +752,10 @@ void PeerLogicValidation::SyncTransaction(const CTransaction& tx, const CBlockIn } } +static CCriticalSection cs_most_recent_block; +static std::shared_ptr most_recent_block; +static uint256 most_recent_block_hash; + void PeerLogicValidation::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& pblock) { CBlockHeaderAndShortTxIDs cmpctblock(*pblock, true); CNetMsgMaker msgMaker(PROTOCOL_VERSION); @@ -766,6 +770,12 @@ void PeerLogicValidation::NewPoWValidBlock(const CBlockIndex *pindex, const std: bool fWitnessEnabled = IsWitnessEnabled(pindex->pprev, Params().GetConsensus()); uint256 hashBlock(pblock->GetHash()); + { + LOCK(cs_most_recent_block); + most_recent_block_hash = hashBlock; + most_recent_block = pblock; + } + connman->ForEachNode([this, &cmpctblock, pindex, &msgMaker, fWitnessEnabled, &hashBlock](CNode* pnode) { // TODO: Avoid the repeated-serialization here if (pnode->nVersion < INVALID_CB_NO_BAN_VERSION || pnode->fDisconnect) @@ -1090,6 +1100,23 @@ uint32_t GetFetchFlags(CNode* pfrom, const CBlockIndex* pprev, const Consensus:: return nFetchFlags; } +inline void static SendBlockTransactions(const CBlock& block, const BlockTransactionsRequest& req, CNode* pfrom, CConnman& connman) { + BlockTransactions resp(req); + for (size_t i = 0; i < req.indexes.size(); i++) { + if (req.indexes[i] >= block.vtx.size()) { + LOCK(cs_main); + Misbehaving(pfrom->GetId(), 100); + LogPrintf("Peer %d sent us a getblocktxn with out-of-bounds tx indices", pfrom->id); + return; + } + resp.txn[i] = block.vtx[req.indexes[i]]; + } + LOCK(cs_main); + CNetMsgMaker msgMaker(pfrom->GetSendVersion()); + int nSendFlags = State(pfrom->GetId())->fWantsCmpctWitness ? 0 : SERIALIZE_TRANSACTION_NO_WITNESS; + connman.PushMessage(pfrom, msgMaker.Make(nSendFlags, NetMsgType::BLOCKTXN, resp)); +} + bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, int64_t nTimeReceived, const CChainParams& chainparams, CConnman& connman, std::atomic& interruptMsgProc) { unsigned int nMaxSendBufferSize = connman.GetSendBufferSize(); @@ -1529,6 +1556,18 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, BlockTransactionsRequest req; vRecv >> req; + std::shared_ptr recent_block; + { + LOCK(cs_most_recent_block); + if (most_recent_block_hash == req.blockhash) + recent_block = most_recent_block; + // Unlock cs_most_recent_block to avoid cs_main lock inversion + } + if (recent_block) { + SendBlockTransactions(*recent_block, req, pfrom, connman); + return true; + } + LOCK(cs_main); BlockMap::iterator it = mapBlockIndex.find(req.blockhash); @@ -1558,17 +1597,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, bool ret = ReadBlockFromDisk(block, it->second, chainparams.GetConsensus()); assert(ret); - BlockTransactions resp(req); - for (size_t i = 0; i < req.indexes.size(); i++) { - if (req.indexes[i] >= block.vtx.size()) { - Misbehaving(pfrom->GetId(), 100); - LogPrintf("Peer %d sent us a getblocktxn with out-of-bounds tx indices", pfrom->id); - return true; - } - resp.txn[i] = block.vtx[req.indexes[i]]; - } - int nSendFlags = State(pfrom->GetId())->fWantsCmpctWitness ? 0 : SERIALIZE_TRANSACTION_NO_WITNESS; - connman.PushMessage(pfrom, msgMaker.Make(nSendFlags, NetMsgType::BLOCKTXN, resp)); + SendBlockTransactions(block, req, pfrom, connman); } From 5749a853b9f46cc58d51aae8b5d4dcbd110a20ca Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 22 Dec 2016 15:29:06 -0800 Subject: [PATCH 08/14] Cache most-recently-connected compact block --- src/net_processing.cpp | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 5de103d4e..669be6e89 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -754,10 +754,11 @@ void PeerLogicValidation::SyncTransaction(const CTransaction& tx, const CBlockIn static CCriticalSection cs_most_recent_block; static std::shared_ptr most_recent_block; +static std::shared_ptr most_recent_compact_block; static uint256 most_recent_block_hash; void PeerLogicValidation::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& pblock) { - CBlockHeaderAndShortTxIDs cmpctblock(*pblock, true); + std::shared_ptr pcmpctblock = std::make_shared (*pblock, true); CNetMsgMaker msgMaker(PROTOCOL_VERSION); LOCK(cs_main); @@ -774,9 +775,10 @@ void PeerLogicValidation::NewPoWValidBlock(const CBlockIndex *pindex, const std: LOCK(cs_most_recent_block); most_recent_block_hash = hashBlock; most_recent_block = pblock; + most_recent_compact_block = pcmpctblock; } - connman->ForEachNode([this, &cmpctblock, pindex, &msgMaker, fWitnessEnabled, &hashBlock](CNode* pnode) { + connman->ForEachNode([this, &pcmpctblock, pindex, &msgMaker, fWitnessEnabled, &hashBlock](CNode* pnode) { // TODO: Avoid the repeated-serialization here if (pnode->nVersion < INVALID_CB_NO_BAN_VERSION || pnode->fDisconnect) return; @@ -789,7 +791,7 @@ void PeerLogicValidation::NewPoWValidBlock(const CBlockIndex *pindex, const std: LogPrint("net", "%s sending header-and-ids %s to peer %d\n", "PeerLogicValidation::NewPoWValidBlock", hashBlock.ToString(), pnode->id); - connman->PushMessage(pnode, msgMaker.Make(NetMsgType::CMPCTBLOCK, cmpctblock)); + connman->PushMessage(pnode, msgMaker.Make(NetMsgType::CMPCTBLOCK, *pcmpctblock)); state.pindexBestHeaderSent = pindex; } }); @@ -2859,13 +2861,24 @@ bool SendMessages(CNode* pto, CConnman& connman, std::atomic& interruptMsg // probably means we're doing an initial-ish-sync or they're slow LogPrint("net", "%s sending header-and-ids %s to peer %d\n", __func__, vHeaders.front().GetHash().ToString(), pto->id); - //TODO: Shouldn't need to reload block from disk, but requires refactor - CBlock block; - bool ret = ReadBlockFromDisk(block, pBestIndex, consensusParams); - assert(ret); - CBlockHeaderAndShortTxIDs cmpctblock(block, state.fWantsCmpctWitness); + int nSendFlags = state.fWantsCmpctWitness ? 0 : SERIALIZE_TRANSACTION_NO_WITNESS; - connman.PushMessage(pto, msgMaker.Make(nSendFlags, NetMsgType::CMPCTBLOCK, cmpctblock)); + + LOCK(cs_most_recent_block); + if (most_recent_block_hash == pBestIndex->GetBlockHash()) { + if (state.fWantsCmpctWitness) + connman.PushMessage(pto, msgMaker.Make(nSendFlags, NetMsgType::CMPCTBLOCK, *most_recent_compact_block)); + else { + CBlockHeaderAndShortTxIDs cmpctblock(*most_recent_block, state.fWantsCmpctWitness); + connman.PushMessage(pto, msgMaker.Make(nSendFlags, NetMsgType::CMPCTBLOCK, cmpctblock)); + } + } else { + CBlock block; + bool ret = ReadBlockFromDisk(block, pBestIndex, consensusParams); + assert(ret); + CBlockHeaderAndShortTxIDs cmpctblock(block, state.fWantsCmpctWitness); + connman.PushMessage(pto, msgMaker.Make(nSendFlags, NetMsgType::CMPCTBLOCK, cmpctblock)); + } state.pindexBestHeaderSent = pBestIndex; } else if (state.fPreferHeaders) { if (vHeaders.size() > 1) { From 9eb67f50008970d09a8253475d591cdad872692e Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sat, 31 Dec 2016 18:16:08 +0100 Subject: [PATCH 09/14] Ensure we meet the BIP 152 old-relay-types response requirements In order to do this, we must call ActivateBestChain prior to responding getdata requests for blocks which we announced using compact blocks. For getheaders responses we dont need code changes, but do note that we must reset the bestHeaderSent so that the SendMessages call re-announces the header in question. While we could do something smarter for getblocks, calling ActivateBestChain is simple and more obviously correct, instead of doing something more similar to getheaders. See-also the BIP clarifications at https://github.com/bitcoin/bips/pull/486 --- src/net_processing.cpp | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 669be6e89..fdb70d657 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -957,6 +957,16 @@ void static ProcessGetData(CNode* pfrom, const Consensus::Params& consensusParam BlockMap::iterator mi = mapBlockIndex.find(inv.hash); if (mi != mapBlockIndex.end()) { + if (mi->second->nChainTx && !mi->second->IsValid(BLOCK_VALID_SCRIPTS) && + mi->second->IsValid(BLOCK_VALID_TREE)) { + // If we have the block and all of its parents, but have not yet validated it, + // we might be in the middle of connecting it (ie in the unlock of cs_main + // before ActivateBestChain but after AcceptBlock). + // In this case, we need to run ActivateBestChain prior to checking the relay + // conditions below. + CValidationState dummy; + ActivateBestChain(dummy, Params()); + } if (chainActive.Contains(mi->second)) { send = true; } else { @@ -1517,6 +1527,18 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, LOCK(cs_main); + // We might have announced the currently-being-connected tip using a + // compact block, which resulted in the peer sending a getblocks + // request, which we would otherwise respond to without the new block. + // To avoid this situation we simply verify that we are on our best + // known chain now. This is super overkill, but we handle it better + // for getheaders requests, and there are no known nodes which support + // compact blocks but still use getblocks to request blocks. + { + CValidationState dummy; + ActivateBestChain(dummy, Params()); + } + // Find the last block the caller has in the main chain const CBlockIndex* pindex = FindForkInGlobalIndex(chainActive, locator); @@ -1647,6 +1669,14 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, // if our peer has chainActive.Tip() (and thus we are sending an empty // headers message). In both cases it's safe to update // pindexBestHeaderSent to be our tip. + // + // It is important that we simply reset the BestHeaderSent value here, + // and not max(BestHeaderSent, newHeaderSent). We might have announced + // the currently-being-connected tip using a compact block, which + // resulted in the peer sending a headers request, which we respond to + // without the new block. By resetting the BestHeaderSent, we ensure we + // will re-announce the new block via headers (or compact blocks again) + // in the SendMessages logic. nodestate->pindexBestHeaderSent = pindex ? pindex : chainActive.Tip(); connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::HEADERS, vHeaders)); } From c1ae4fcf7d5d85c182e36ff6f7a529f8a84aa372 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 5 Jan 2017 15:15:40 -0500 Subject: [PATCH 10/14] Avoid holding cs_most_recent_block while calling ReadBlockFromDisk --- src/net_processing.cpp | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index fdb70d657..440696ae5 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2894,15 +2894,20 @@ bool SendMessages(CNode* pto, CConnman& connman, std::atomic& interruptMsg int nSendFlags = state.fWantsCmpctWitness ? 0 : SERIALIZE_TRANSACTION_NO_WITNESS; - LOCK(cs_most_recent_block); - if (most_recent_block_hash == pBestIndex->GetBlockHash()) { - if (state.fWantsCmpctWitness) - connman.PushMessage(pto, msgMaker.Make(nSendFlags, NetMsgType::CMPCTBLOCK, *most_recent_compact_block)); - else { - CBlockHeaderAndShortTxIDs cmpctblock(*most_recent_block, state.fWantsCmpctWitness); - connman.PushMessage(pto, msgMaker.Make(nSendFlags, NetMsgType::CMPCTBLOCK, cmpctblock)); + bool fGotBlockFromCache = false; + { + LOCK(cs_most_recent_block); + if (most_recent_block_hash == pBestIndex->GetBlockHash()) { + if (state.fWantsCmpctWitness) + connman.PushMessage(pto, msgMaker.Make(nSendFlags, NetMsgType::CMPCTBLOCK, *most_recent_compact_block)); + else { + CBlockHeaderAndShortTxIDs cmpctblock(*most_recent_block, state.fWantsCmpctWitness); + connman.PushMessage(pto, msgMaker.Make(nSendFlags, NetMsgType::CMPCTBLOCK, cmpctblock)); + } + fGotBlockFromCache = true; } - } else { + } + if (!fGotBlockFromCache) { CBlock block; bool ret = ReadBlockFromDisk(block, pBestIndex, consensusParams); assert(ret); From 0df777db6d62a4fc3353edecdcc0094dc1abbd18 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 11 Jan 2017 14:47:52 -0800 Subject: [PATCH 11/14] Use a temp pindex to avoid a const_cast in ProcessNewBlockHeaders --- src/validation.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 20d75d957..5f0cf10ce 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3035,11 +3035,13 @@ bool ProcessNewBlockHeaders(const std::vector& headers, CValidatio { LOCK(cs_main); for (const CBlockHeader& header : headers) { - // cast away the ppindex-returns-const CBlockIndex - we're just assigning it to a CBlockIndex* - // that we own and is updated non-const anyway - if (!AcceptBlockHeader(header, state, chainparams, const_cast(ppindex))) { + CBlockIndex *pindex = NULL; // Use a temp pindex instead of ppindex to avoid a const_cast + if (!AcceptBlockHeader(header, state, chainparams, &pindex)) { return false; } + if (ppindex) { + *ppindex = pindex; + } } } NotifyHeaderTip(); From 962f7f054fd9692f92d40c771cf3ba8ef0cde891 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 11 Jan 2017 21:15:28 -0800 Subject: [PATCH 12/14] Call ActivateBestChain without cs_main/with most_recent_block There is still a call to ActivateBestChain with cs_main if a peer requests the block prior to it being validated, but this one is more specifically-gated, so should be less of an issue. --- src/net_processing.cpp | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 440696ae5..015921ebe 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -964,8 +964,13 @@ void static ProcessGetData(CNode* pfrom, const Consensus::Params& consensusParam // before ActivateBestChain but after AcceptBlock). // In this case, we need to run ActivateBestChain prior to checking the relay // conditions below. + std::shared_ptr a_recent_block; + { + LOCK(cs_most_recent_block); + a_recent_block = most_recent_block; + } CValidationState dummy; - ActivateBestChain(dummy, Params()); + ActivateBestChain(dummy, Params(), a_recent_block); } if (chainActive.Contains(mi->second)) { send = true; @@ -1525,8 +1530,6 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, uint256 hashStop; vRecv >> locator >> hashStop; - LOCK(cs_main); - // We might have announced the currently-being-connected tip using a // compact block, which resulted in the peer sending a getblocks // request, which we would otherwise respond to without the new block. @@ -1535,10 +1538,17 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, // for getheaders requests, and there are no known nodes which support // compact blocks but still use getblocks to request blocks. { + std::shared_ptr a_recent_block; + { + LOCK(cs_most_recent_block); + a_recent_block = most_recent_block; + } CValidationState dummy; - ActivateBestChain(dummy, Params()); + ActivateBestChain(dummy, Params(), a_recent_block); } + LOCK(cs_main); + // Find the last block the caller has in the main chain const CBlockIndex* pindex = FindForkInGlobalIndex(chainActive, locator); From 73666ad05932429c860efe74eb388d212c152fc4 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 12 Jan 2017 12:15:17 -0800 Subject: [PATCH 13/14] Add comment to describe callers to ActivateBestChain --- src/validation.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/validation.cpp b/src/validation.cpp index 5f0cf10ce..a26426be5 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2388,6 +2388,11 @@ static void NotifyHeaderTip() { * that is already loaded (to avoid loading it again from disk). */ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams, std::shared_ptr pblock) { + // Note that while we're often called here from ProcessNewBlock, this is + // far from a guarantee. Things in the P2P/RPC will often end up calling + // us in the middle of ProcessNewBlock - do not assume pblock is set + // sanely for performance or correctness! + CBlockIndex *pindexMostWork = NULL; CBlockIndex *pindexNewTip = NULL; do { From 02ee4eb2637db9077aefa1f5e59864218e016b93 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 13 Jan 2017 15:49:31 -0500 Subject: [PATCH 14/14] Make most_recent_compact_block a pointer to a const --- src/net_processing.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 015921ebe..8bcb59067 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -754,11 +754,11 @@ void PeerLogicValidation::SyncTransaction(const CTransaction& tx, const CBlockIn static CCriticalSection cs_most_recent_block; static std::shared_ptr most_recent_block; -static std::shared_ptr most_recent_compact_block; +static std::shared_ptr most_recent_compact_block; static uint256 most_recent_block_hash; void PeerLogicValidation::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& pblock) { - std::shared_ptr pcmpctblock = std::make_shared (*pblock, true); + std::shared_ptr pcmpctblock = std::make_shared (*pblock, true); CNetMsgMaker msgMaker(PROTOCOL_VERSION); LOCK(cs_main);