Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Relay compact block messages prior to full block connection #9375

Merged
merged 14 commits into from Jan 13, 2017
Merged

Relay compact block messages prior to full block connection #9375

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
8017547
Make CBlockIndex*es in net_processing const
TheBlueMatt Dec 19, 2016
9a0b2f4
[qa] Make compact blocks test construction using fetch methods
TheBlueMatt Jan 4, 2017
8baaba6
[qa] Avoid race in preciousblock test.
TheBlueMatt Dec 19, 2016
180586f
Call AcceptBlock with the block's shared_ptr instead of CBlock&
TheBlueMatt Dec 19, 2016
6987219
Add a CValidationInterface::NewPoWValidBlock callback
TheBlueMatt Dec 19, 2016
c802092
Relay compact block messages prior to full block connection
TheBlueMatt Jan 5, 2017
9eaec08
Cache most-recently-announced block's shared_ptr
TheBlueMatt Dec 15, 2016
5749a85
Cache most-recently-connected compact block
TheBlueMatt Dec 22, 2016
9eb67f5
Ensure we meet the BIP 152 old-relay-types response requirements
TheBlueMatt Dec 31, 2016
c1ae4fc
Avoid holding cs_most_recent_block while calling ReadBlockFromDisk
TheBlueMatt Jan 5, 2017
0df777d
Use a temp pindex to avoid a const_cast in ProcessNewBlockHeaders
TheBlueMatt Jan 11, 2017
962f7f0
Call ActivateBestChain without cs_main/with most_recent_block
TheBlueMatt Jan 12, 2017
73666ad
Add comment to describe callers to ActivateBestChain
TheBlueMatt Jan 12, 2017
02ee4eb
Make most_recent_compact_block a pointer to a const
TheBlueMatt Jan 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 19 additions & 7 deletions qa/rpc-tests/p2p-compactblocks.py
View file
Open in desktop
Expand Up @@ -310,6 +310,9 @@ def test_compactblock_construction(self, node, test_node, version, use_witness_a
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)
Expand All @@ -319,27 +322,36 @@ def test_compactblock_construction(self, node, test_node, version, use_witness_a
[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)
Expand Down
2 changes: 1 addition & 1 deletion qa/rpc-tests/preciousblock.py
View file
Open in desktop
Expand Up @@ -102,7 +102,7 @@ def run_test(self):
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)
Expand Down
157 changes: 116 additions & 41 deletions src/net_processing.cpp
View file
Open in desktop

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions src/net_processing.h
View file
Open in desktop
Expand Up @@ -24,6 +24,7 @@ class PeerLogicValidation : public CValidationInterface {
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<const CBlock>& pblock);
};

struct CNodeStateStats {
Expand Down
31 changes: 21 additions & 10 deletions src/validation.cpp
View file
Open in desktop
Expand Up @@ -3030,12 +3030,14 @@ static bool AcceptBlockHeader(const CBlockHeader& block, CValidationState& state
}

// Exposed wrapper for AcceptBlockHeader
bool ProcessNewBlockHeaders(const std::vector<CBlockHeader>& headers, CValidationState& state, const CChainParams& chainparams, CBlockIndex** ppindex)
bool ProcessNewBlockHeaders(const std::vector<CBlockHeader>& 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*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this is non-obvious to a caller. And it's especially bad that one of them relies on the const not actually being const :(

Maybe return an updated index, or null in the case of failure? Or return a pair?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really? I dont see anything non-obvious here? The caller passes in a reference to a const CBlockIndex*, and the function sets that element to the new CBlockIndex*, ie it is returning a const CBlockIndex*, not editing a CBlockIndex which was passed in.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid the const cast, you could do something like:

            CBlockIndex* pindex = nullptr;
            if (!AcceptBlockHeader(header, state, chainparams, &pindex)) {
                return false;
            }
            *ppindex = pindex;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer @ryanofsky's code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, changed.

// that we own and is updated non-const anyway
if (!AcceptBlockHeader(header, state, chainparams, const_cast<CBlockIndex**>(ppindex))) {
return false;
}
}
Expand All @@ -3045,8 +3047,10 @@ bool ProcessNewBlockHeaders(const std::vector<CBlockHeader>& 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<const CBlock>& 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);

Expand Down Expand Up @@ -3092,6 +3096,11 @@ static bool AcceptBlock(const CBlock& block, CValidationState& state, const CCha
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
Expand Down Expand Up @@ -3126,7 +3135,7 @@ bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<cons
CBlockIndex *pindex = NULL;
if (fNewBlock) *fNewBlock = false;
CValidationState state;
bool ret = AcceptBlock(*pblock, state, chainparams, &pindex, fForceProcessing, NULL, fNewBlock);
bool ret = AcceptBlock(pblock, state, chainparams, &pindex, fForceProcessing, NULL, fNewBlock);
CheckBlockIndex(chainparams.GetConsensus());
if (!ret) {
GetMainSignals().BlockChecked(*pblock, state);
Expand Down Expand Up @@ -3753,7 +3762,8 @@ bool LoadExternalBlockFile(const CChainParams& chainparams, FILE* fileIn, CDiskB
dbp->nPos = nBlockPos;
blkdat.SetLimit(nBlockPos + nSize);
blkdat.SetPos(nBlockPos);
CBlock block;
std::shared_ptr<CBlock> pblock = std::make_shared<CBlock>();
CBlock& block = *pblock;
blkdat >> block;
nRewind = blkdat.GetPos();

Expand All @@ -3771,7 +3781,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;
Expand All @@ -3798,16 +3808,17 @@ bool LoadExternalBlockFile(const CChainParams& chainparams, FILE* fileIn, CDiskB
std::pair<std::multimap<uint256, CDiskBlockPos>::iterator, std::multimap<uint256, CDiskBlockPos>::iterator> range = mapBlocksUnknownParent.equal_range(head);
while (range.first != range.second) {
std::multimap<uint256, CDiskBlockPos>::iterator it = range.first;
if (ReadBlockFromDisk(block, it->second, chainparams.GetConsensus()))
std::shared_ptr<CBlock> pblockrecursive = std::make_shared<CBlock>();
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++;
Expand Down
2 changes: 1 addition & 1 deletion src/validation.h
View file
Open in desktop
Expand Up @@ -243,7 +243,7 @@ bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<cons
* @param[in] chainparams The params for the chain we want to connect to
* @param[out] ppindex If set, the pointer will be set to point to the last new block index object for the given headers
*/
bool ProcessNewBlockHeaders(const std::vector<CBlockHeader>& block, CValidationState& state, const CChainParams& chainparams, CBlockIndex** ppindex=NULL);
bool ProcessNewBlockHeaders(const std::vector<CBlockHeader>& 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);
Expand Down
3 changes: 3 additions & 0 deletions src/validationinterface.cpp
View file
Open in desktop
Expand Up @@ -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) {
Expand All @@ -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() {
Expand All @@ -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();
}
6 changes: 6 additions & 0 deletions src/validationinterface.h
View file
Open in desktop
Expand Up @@ -8,6 +8,7 @@

#include <boost/signals2/signal.hpp>
#include <boost/shared_ptr.hpp>
#include <memory>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This include is unnecessary

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should be relying on indirect includes, and this file does directly use shared_ptr.


class CBlock;
class CBlockIndex;
Expand Down Expand Up @@ -40,6 +41,7 @@ class CValidationInterface {
virtual void BlockChecked(const CBlock&, const CValidationState&) {}
virtual void GetScriptForMining(boost::shared_ptr<CReserveScript>&) {};
virtual void ResetRequestCount(const uint256 &hash) {};
virtual void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr<const CBlock>& block) {};
friend void ::RegisterValidationInterface(CValidationInterface*);
friend void ::UnregisterValidationInterface(CValidationInterface*);
friend void ::UnregisterAllValidationInterfaces();
Expand All @@ -66,6 +68,10 @@ struct CMainSignals {
boost::signals2::signal<void (boost::shared_ptr<CReserveScript>&)> ScriptForMining;
/** Notifies listeners that a block has been successfully mined */
boost::signals2::signal<void (const uint256 &)> 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<void (const CBlockIndex *, const std::shared_ptr<const CBlock>&)> NewPoWValidBlock;
};

CMainSignals& GetMainSignals();
Expand Down