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.

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

net: Serve blocks directly from disk when possible #13151

Merged
merged 1 commit into from May 23, 2018
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.
+107 −47
Diff settings

Always

Just for now

Copy path View file
@@ -1070,12 +1070,13 @@ static void RelayAddress(const CAddress& addr, bool fReachable, CConnman* connma
connman->ForEachNodeThen(std::move(sortfunc), std::move(pushfunc));
}

void static ProcessGetBlockData(CNode* pfrom, const Consensus::Params& consensusParams, const CInv& inv, CConnman* connman, const std::atomic<bool>& interruptMsgProc)
void static ProcessGetBlockData(CNode* pfrom, const CChainParams& chainparams, const CInv& inv, CConnman* connman, const std::atomic<bool>& interruptMsgProc)
{
bool send = false;
std::shared_ptr<const CBlock> a_recent_block;
std::shared_ptr<const CBlockHeaderAndShortTxIDs> a_recent_compact_block;
bool fWitnessesPresentInARecentCompactBlock;
const Consensus::Params& consensusParams = chainparams.GetConsensus();
{
LOCK(cs_most_recent_block);
a_recent_block = most_recent_block;
@@ -1142,60 +1143,71 @@ void static ProcessGetBlockData(CNode* pfrom, const Consensus::Params& consensus
std::shared_ptr<const CBlock> pblock;
if (a_recent_block && a_recent_block->GetHash() == pindex->GetBlockHash()) {
pblock = a_recent_block;
} else if (inv.type == MSG_WITNESS_BLOCK) {

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke May 2, 2018

Member

Shouldn't this compare against the serialization flags of the block on disk? Currently you are assuming that all blocks are serialized as witness blocks on disk, but this is not true for all "early" blocks.

This comment has been minimized.

Copy link
@laanwj

laanwj May 2, 2018

Author Member

Yes I'm not convinced this logic is correct. It seems to work, though, even for the initial blocks.

Edit: What is the operation to convert from a non-witness block to witness block with no witnesses? I suppose this could still be done without a full round-trip?

This comment has been minimized.

Copy link
@sipa

sipa May 2, 2018

Member

@laanwj It's always correct to give the raw blocks we store to peers that ask for witnesses (even if the block does not have a witness).

Converting extended format to basic format is a lot more complicated. You could have a special CTransaction which skips the witness fields instead of reading/deserializing them, but I don't see how to do it without going through some form of serialization code.

This comment has been minimized.

Copy link
@laanwj

laanwj May 2, 2018

Author Member

@sipa Thanks.

Converting extended format to basic format is a lot more complicated. You could have a special CTransaction which skips the witness fields instead of reading/deserializing them, but I don't see how to do it without going through some form of serialization code.

Right, that case should fall back to deserialization->serialization right now. I don't think we can do much better there. Not sure it's even worth optimizing, there won't be many new clients being synced with pre-segwit versions.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke May 2, 2018

Member

Leaving out the less common edge case (non-witness peers) seems fine for now.

This comment has been minimized.

Copy link
@sipa

sipa May 13, 2018

Member

Unsure if we care, but we could also check whether SegWit was active for the requested block, and if not, we can serve without deserialization even when witnesses are not requested.

This comment has been minimized.

Copy link
@laanwj

laanwj May 14, 2018

Author Member

@sipa Yes, that would be something that could be done here.

// Fast-path: in this case it is possible to serve the block directly from disk,
// as the network format matches the format on disk
std::vector<uint8_t> block_data;
if (!ReadRawBlockFromDisk(block_data, pindex, chainparams.MessageStart())) {
assert(!"cannot load block from disk");
}
connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::BLOCK, MakeSpan(block_data)));
// Don't set pblock as we've sent the block
} else {
// Send block from disk
std::shared_ptr<CBlock> pblockRead = std::make_shared<CBlock>();
if (!ReadBlockFromDisk(*pblockRead, pindex, consensusParams))
assert(!"cannot load block from disk");
pblock = pblockRead;
}
if (inv.type == MSG_BLOCK)
connman->PushMessage(pfrom, msgMaker.Make(SERIALIZE_TRANSACTION_NO_WITNESS, NetMsgType::BLOCK, *pblock));
else if (inv.type == MSG_WITNESS_BLOCK)
connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::BLOCK, *pblock));
else if (inv.type == MSG_FILTERED_BLOCK)
{
bool sendMerkleBlock = false;
CMerkleBlock merkleBlock;
if (pblock) {
if (inv.type == MSG_BLOCK)
connman->PushMessage(pfrom, msgMaker.Make(SERIALIZE_TRANSACTION_NO_WITNESS, NetMsgType::BLOCK, *pblock));
else if (inv.type == MSG_WITNESS_BLOCK)
connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::BLOCK, *pblock));
else if (inv.type == MSG_FILTERED_BLOCK)
{
LOCK(pfrom->cs_filter);
if (pfrom->pfilter) {
sendMerkleBlock = true;
merkleBlock = CMerkleBlock(*pblock, *pfrom->pfilter);
bool sendMerkleBlock = false;
CMerkleBlock merkleBlock;
{
LOCK(pfrom->cs_filter);
if (pfrom->pfilter) {
sendMerkleBlock = true;
merkleBlock = CMerkleBlock(*pblock, *pfrom->pfilter);
}
}
if (sendMerkleBlock) {
connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::MERKLEBLOCK, merkleBlock));
// CMerkleBlock just contains hashes, so also push any transactions in the block the client did not see
// This avoids hurting performance by pointlessly requiring a round-trip
// Note that there is currently no way for a node to request any single transactions we didn't send here -
// they must either disconnect and retry or request the full block.
// Thus, the protocol spec specified allows for us to provide duplicate txn here,
// however we MUST always provide at least what the remote peer needs
typedef std::pair<unsigned int, uint256> PairType;
for (PairType& pair : merkleBlock.vMatchedTxn)
connman->PushMessage(pfrom, msgMaker.Make(SERIALIZE_TRANSACTION_NO_WITNESS, NetMsgType::TX, *pblock->vtx[pair.first]));
}
// else
// no response
}
if (sendMerkleBlock) {
connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::MERKLEBLOCK, merkleBlock));
// CMerkleBlock just contains hashes, so also push any transactions in the block the client did not see
// This avoids hurting performance by pointlessly requiring a round-trip
// Note that there is currently no way for a node to request any single transactions we didn't send here -
// they must either disconnect and retry or request the full block.
// Thus, the protocol spec specified allows for us to provide duplicate txn here,
// however we MUST always provide at least what the remote peer needs
typedef std::pair<unsigned int, uint256> PairType;
for (PairType& pair : merkleBlock.vMatchedTxn)
connman->PushMessage(pfrom, msgMaker.Make(SERIALIZE_TRANSACTION_NO_WITNESS, NetMsgType::TX, *pblock->vtx[pair.first]));
}
// else
// no response
}
else if (inv.type == MSG_CMPCT_BLOCK)
{
// If a peer is asking for old blocks, we're almost guaranteed
// they won't have a useful mempool to match against a compact block,
// and we don't feel like constructing the object for them, so
// instead we respond with the full, non-compact block.
bool fPeerWantsWitness = State(pfrom->GetId())->fWantsCmpctWitness;
int nSendFlags = fPeerWantsWitness ? 0 : SERIALIZE_TRANSACTION_NO_WITNESS;
if (CanDirectFetch(consensusParams) && pindex->nHeight >= chainActive.Height() - MAX_CMPCTBLOCK_DEPTH) {
if ((fPeerWantsWitness || !fWitnessesPresentInARecentCompactBlock) && a_recent_compact_block && a_recent_compact_block->header.GetHash() == pindex->GetBlockHash()) {
connman->PushMessage(pfrom, msgMaker.Make(nSendFlags, NetMsgType::CMPCTBLOCK, *a_recent_compact_block));
else if (inv.type == MSG_CMPCT_BLOCK)
{
// If a peer is asking for old blocks, we're almost guaranteed
// they won't have a useful mempool to match against a compact block,
// and we don't feel like constructing the object for them, so
// instead we respond with the full, non-compact block.
bool fPeerWantsWitness = State(pfrom->GetId())->fWantsCmpctWitness;
int nSendFlags = fPeerWantsWitness ? 0 : SERIALIZE_TRANSACTION_NO_WITNESS;
if (CanDirectFetch(consensusParams) && pindex->nHeight >= chainActive.Height() - MAX_CMPCTBLOCK_DEPTH) {
if ((fPeerWantsWitness || !fWitnessesPresentInARecentCompactBlock) && a_recent_compact_block && a_recent_compact_block->header.GetHash() == pindex->GetBlockHash()) {
connman->PushMessage(pfrom, msgMaker.Make(nSendFlags, NetMsgType::CMPCTBLOCK, *a_recent_compact_block));
} else {
CBlockHeaderAndShortTxIDs cmpctblock(*pblock, fPeerWantsWitness);
connman->PushMessage(pfrom, msgMaker.Make(nSendFlags, NetMsgType::CMPCTBLOCK, cmpctblock));
}
} else {
CBlockHeaderAndShortTxIDs cmpctblock(*pblock, fPeerWantsWitness);
connman->PushMessage(pfrom, msgMaker.Make(nSendFlags, NetMsgType::CMPCTBLOCK, cmpctblock));
connman->PushMessage(pfrom, msgMaker.Make(nSendFlags, NetMsgType::BLOCK, *pblock));
}
} else {
connman->PushMessage(pfrom, msgMaker.Make(nSendFlags, NetMsgType::BLOCK, *pblock));
}
}

@@ -1213,7 +1225,7 @@ void static ProcessGetBlockData(CNode* pfrom, const Consensus::Params& consensus
}
}

void static ProcessGetData(CNode* pfrom, const Consensus::Params& consensusParams, CConnman* connman, const std::atomic<bool>& interruptMsgProc)
void static ProcessGetData(CNode* pfrom, const CChainParams& chainparams, CConnman* connman, const std::atomic<bool>& interruptMsgProc)
{
AssertLockNotHeld(cs_main);

@@ -1262,7 +1274,7 @@ void static ProcessGetData(CNode* pfrom, const Consensus::Params& consensusParam
const CInv &inv = *it;
if (inv.type == MSG_BLOCK || inv.type == MSG_FILTERED_BLOCK || inv.type == MSG_CMPCT_BLOCK || inv.type == MSG_WITNESS_BLOCK) {
it++;
ProcessGetBlockData(pfrom, consensusParams, inv, connman, interruptMsgProc);
ProcessGetBlockData(pfrom, chainparams, inv, connman, interruptMsgProc);
}
}

@@ -1971,7 +1983,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
}

pfrom->vRecvGetData.insert(pfrom->vRecvGetData.end(), vInv.begin(), vInv.end());
ProcessGetData(pfrom, chainparams.GetConsensus(), connman, interruptMsgProc);
ProcessGetData(pfrom, chainparams, connman, interruptMsgProc);
}


@@ -2941,7 +2953,7 @@ bool PeerLogicValidation::ProcessMessages(CNode* pfrom, std::atomic<bool>& inter
bool fMoreWork = false;

if (!pfrom->vRecvGetData.empty())
ProcessGetData(pfrom, chainparams.GetConsensus(), connman, interruptMsgProc);
ProcessGetData(pfrom, chainparams, connman, interruptMsgProc);

if (pfrom->fDisconnect)
return false;
Copy path View file
@@ -1125,6 +1125,52 @@ bool ReadBlockFromDisk(CBlock& block, const CBlockIndex* pindex, const Consensus
return true;
}

bool ReadRawBlockFromDisk(std::vector<uint8_t>& block, const CDiskBlockPos& pos, const CMessageHeader::MessageStartChars& message_start)
{
CDiskBlockPos hpos = pos;
hpos.nPos -= 8; // Seek back 8 bytes for meta header
CAutoFile filein(OpenBlockFile(hpos, true), SER_DISK, CLIENT_VERSION);
if (filein.IsNull()) {
return error("%s: OpenBlockFile failed for %s", __func__, pos.ToString());
}

try {
CMessageHeader::MessageStartChars blk_start;
unsigned int blk_size;

filein >> blk_start >> blk_size;

if (memcmp(blk_start, message_start, CMessageHeader::MESSAGE_START_SIZE)) {
return error("%s: Block magic mismatch for %s: %s versus expected %s", __func__, pos.ToString(),
HexStr(blk_start, blk_start + CMessageHeader::MESSAGE_START_SIZE),
HexStr(message_start, message_start + CMessageHeader::MESSAGE_START_SIZE));
}

if (blk_size > MAX_SIZE) {
return error("%s: Block data is larger than maximum deserialization size for %s: %s versus %s", __func__, pos.ToString(),
blk_size, MAX_SIZE);
}

block.resize(blk_size); // Zeroing of memory is intentional here

This comment has been minimized.

Copy link
@promag

promag May 7, 2018

Member

Zeroing of memory is intentional

Why?

This comment has been minimized.

Copy link
@laanwj

laanwj May 7, 2018

Author Member

To avoid heartbleed-type leaks as this data goes directly over the network.

filein.read((char*)block.data(), blk_size);
} catch(const std::exception& e) {

This comment has been minimized.

Copy link
@promag

promag May 7, 2018

Member

nit, space after catch } catch (....

return error("%s: Read from block file failed: %s for %s", __func__, e.what(), pos.ToString());
}

return true;
}

bool ReadRawBlockFromDisk(std::vector<uint8_t>& block, const CBlockIndex* pindex, const CMessageHeader::MessageStartChars& message_start)
{
CDiskBlockPos block_pos;
{
LOCK(cs_main);
block_pos = pindex->GetBlockPos();
}

return ReadRawBlockFromDisk(block, block_pos, message_start);
}

CAmount GetBlockSubsidy(int nHeight, const Consensus::Params& consensusParams)
{
int halvings = nHeight / consensusParams.nSubsidyHalvingInterval;
Copy path View file
@@ -398,6 +398,8 @@ void InitScriptExecutionCache();
/** Functions for disk access for blocks */
bool ReadBlockFromDisk(CBlock& block, const CDiskBlockPos& pos, const Consensus::Params& consensusParams);
bool ReadBlockFromDisk(CBlock& block, const CBlockIndex* pindex, const Consensus::Params& consensusParams);
bool ReadRawBlockFromDisk(std::vector<uint8_t>& block, const CDiskBlockPos& pos, const CMessageHeader::MessageStartChars& message_start);
bool ReadRawBlockFromDisk(std::vector<uint8_t>& block, const CBlockIndex* pindex, const CMessageHeader::MessageStartChars& message_start);

/** Functions for validating blocks and updating the block tree */

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.