Skip to content

Commit 6aa28ab

Browse files
sipasipa
committed
Use cmpctblock type 2 for segwit-enabled transfer
Contains version negotiation logic by Matt Corallo and bugfixes by Suhas Daftuar.
1 parent be7555f commit 6aa28ab

File tree

6 files changed

+64
-30
lines changed

6 files changed

+64
-30
lines changed

‎src/blockencodings.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,15 +17,15 @@
1717

1818
#define MIN_TRANSACTION_BASE_SIZE (::GetSerializeSize(CTransaction(), SER_NETWORK, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS))
1919

20-
CBlockHeaderAndShortTxIDs::CBlockHeaderAndShortTxIDs(const CBlock& block) :
20+
CBlockHeaderAndShortTxIDs::CBlockHeaderAndShortTxIDs(const CBlock& block, bool fUseWTXID) :
2121
nonce(GetRand(std::numeric_limits<uint64_t>::max())),
2222
shorttxids(block.vtx.size() - 1), prefilledtxn(1), header(block) {
2323
FillShortTxIDSelector();
2424
//TODO: Use our mempool prior to block acceptance to predictively fill more than just the coinbase
2525
prefilledtxn[0] = {0, block.vtx[0]};
2626
for (size_t i = 1; i < block.vtx.size(); i++) {
2727
const CTransaction& tx = block.vtx[i];
28-
shorttxids[i - 1] = GetShortID(tx.GetHash());
28+
shorttxids[i - 1] = GetShortID(fUseWTXID ? tx.GetWitnessHash() : tx.GetHash());
2929
}
3030
}
3131

‎src/blockencodings.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,7 @@ class CBlockHeaderAndShortTxIDs {
146146
// Dummy for deserialization
147147
CBlockHeaderAndShortTxIDs() {}
148148

149-
CBlockHeaderAndShortTxIDs(const CBlock& block);
149+
CBlockHeaderAndShortTxIDs(const CBlock& block, bool fUseWTXID);
150150

151151
uint64_t GetShortID(const uint256& txhash) const;
152152

‎src/main.cpp‎

Lines changed: 56 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -289,10 +289,21 @@ struct CNodeState {
289289
bool fPreferHeaders;
290290
//! Whether this peer wants invs or cmpctblocks (when possible) for block announcements.
291291
bool fPreferHeaderAndIDs;
292-
//! Whether this peer will send us cmpctblocks if we request them
292+
/**
293+
* Whether this peer will send us cmpctblocks if we request them.
294+
* This is not used to gate request logic, as we really only care about fSupportsDesiredCmpctVersion,
295+
* but is used as a flag to "lock in" the version of compact blocks (fWantsCmpctWitness) we send.
296+
*/
293297
bool fProvidesHeaderAndIDs;
294298
//! Whether this peer can give us witnesses
295299
bool fHaveWitness;
300+
//! Whether this peer wants witnesses in cmpctblocks/blocktxns
301+
bool fWantsCmpctWitness;
302+
/**
303+
* If we've announced NODE_WITNESS to this peer: whether the peer sends witnesses in cmpctblocks/blocktxns,
304+
* otherwise: whether this peer sends non-witnesses in cmpctblocks/blocktxns.
305+
*/
306+
bool fSupportsDesiredCmpctVersion;
296307

297308
CNodeState() {
298309
fCurrentlyConnected = false;
@@ -313,6 +324,8 @@ struct CNodeState {
313324
fPreferHeaderAndIDs = false;
314325
fProvidesHeaderAndIDs = false;
315326
fHaveWitness = false;
327+
fWantsCmpctWitness = false;
328+
fSupportsDesiredCmpctVersion = false;
316329
}
317330
};
318331

@@ -467,16 +480,16 @@ void UpdateBlockAvailability(NodeId nodeid, const uint256 &hash) {
467480
}
468481

469482
void MaybeSetPeerAsAnnouncingHeaderAndIDs(const CNodeState* nodestate, CNode* pfrom, CConnman& connman) {
470-
if (pfrom->GetLocalServices() & NODE_WITNESS) {
471-
// Don't ever request compact blocks when segwit is enabled.
483+
if (!nodestate->fSupportsDesiredCmpctVersion) {
484+
// Never ask from peers who can't provide witnesses.
472485
return;
473486
}
474487
if (nodestate->fProvidesHeaderAndIDs) {
475488
BOOST_FOREACH(const NodeId nodeid, lNodesAnnouncingHeaderAndIDs)
476489
if (nodeid == pfrom->GetId())
477490
return;
478491
bool fAnnounceUsingCMPCTBLOCK = false;
479-
uint64_t nCMPCTBLOCKVersion = 1;
492+
uint64_t nCMPCTBLOCKVersion = (pfrom->GetLocalServices() & NODE_WITNESS) ? 2 : 1;
480493
if (lNodesAnnouncingHeaderAndIDs.size() >= 3) {
481494
// As per BIP152, we only get 3 of our peers to announce
482495
// blocks using compact encodings.
@@ -4856,11 +4869,12 @@ void static ProcessGetData(CNode* pfrom, const Consensus::Params& consensusParam
48564869
// they wont have a useful mempool to match against a compact block,
48574870
// and we don't feel like constructing the object for them, so
48584871
// instead we respond with the full, non-compact block.
4872+
bool fPeerWantsWitness = State(pfrom->GetId())->fWantsCmpctWitness;
48594873
if (mi->second->nHeight >= chainActive.Height() - 10) {
4860-
CBlockHeaderAndShortTxIDs cmpctblock(block);
4861-
pfrom->PushMessageWithFlag(SERIALIZE_TRANSACTION_NO_WITNESS, NetMsgType::CMPCTBLOCK, cmpctblock);
4874+
CBlockHeaderAndShortTxIDs cmpctblock(block, fPeerWantsWitness);
4875+
pfrom->PushMessageWithFlag(fPeerWantsWitness ? 0 : SERIALIZE_TRANSACTION_NO_WITNESS, NetMsgType::CMPCTBLOCK, cmpctblock);
48624876
} else
4863-
pfrom->PushMessageWithFlag(SERIALIZE_TRANSACTION_NO_WITNESS, NetMsgType::BLOCK, block);
4877+
pfrom->PushMessageWithFlag(fPeerWantsWitness ? 0 : SERIALIZE_TRANSACTION_NO_WITNESS, NetMsgType::BLOCK, block);
48644878
}
48654879

48664880
// Trigger the peer node to send a getblocks request for the next batch of inventory
@@ -5128,13 +5142,16 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
51285142
pfrom->PushMessage(NetMsgType::SENDHEADERS);
51295143
}
51305144
if (pfrom->nVersion >= SHORT_IDS_BLOCKS_VERSION) {
5131-
// Tell our peer we are willing to provide version-1 cmpctblocks
5145+
// Tell our peer we are willing to provide version 1 or 2 cmpctblocks
51325146
// However, we do not request new block announcements using
51335147
// cmpctblock messages.
51345148
// We send this to non-NODE NETWORK peers as well, because
51355149
// they may wish to request compact blocks from us
51365150
bool fAnnounceUsingCMPCTBLOCK = false;
5137-
uint64_t nCMPCTBLOCKVersion = 1;
5151+
uint64_t nCMPCTBLOCKVersion = 2;
5152+
if (pfrom->GetLocalServices() & NODE_WITNESS)
5153+
pfrom->PushMessage(NetMsgType::SENDCMPCT, fAnnounceUsingCMPCTBLOCK, nCMPCTBLOCKVersion);
5154+
nCMPCTBLOCKVersion = 1;
51385155
pfrom->PushMessage(NetMsgType::SENDCMPCT, fAnnounceUsingCMPCTBLOCK, nCMPCTBLOCKVersion);
51395156
}
51405157
}
@@ -5195,12 +5212,23 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
51955212
else if (strCommand == NetMsgType::SENDCMPCT)
51965213
{
51975214
bool fAnnounceUsingCMPCTBLOCK = false;
5198-
uint64_t nCMPCTBLOCKVersion = 1;
5215+
uint64_t nCMPCTBLOCKVersion = 0;
51995216
vRecv >> fAnnounceUsingCMPCTBLOCK >> nCMPCTBLOCKVersion;
5200-
if (nCMPCTBLOCKVersion == 1) {
5217+
if (nCMPCTBLOCKVersion == 1 || ((pfrom->GetLocalServices() & NODE_WITNESS) && nCMPCTBLOCKVersion == 2)) {
52015218
LOCK(cs_main);
5202-
State(pfrom->GetId())->fProvidesHeaderAndIDs = true;
5203-
State(pfrom->GetId())->fPreferHeaderAndIDs = fAnnounceUsingCMPCTBLOCK;
5219+
// fProvidesHeaderAndIDs is used to "lock in" version of compact blocks we send (fWantsCmpctWitness)
5220+
if (!State(pfrom->GetId())->fProvidesHeaderAndIDs) {
5221+
State(pfrom->GetId())->fProvidesHeaderAndIDs = true;
5222+
State(pfrom->GetId())->fWantsCmpctWitness = nCMPCTBLOCKVersion == 2;
5223+
}
5224+
if (State(pfrom->GetId())->fWantsCmpctWitness == (nCMPCTBLOCKVersion == 2)) // ignore later version announces
5225+
State(pfrom->GetId())->fPreferHeaderAndIDs = fAnnounceUsingCMPCTBLOCK;
5226+
if (!State(pfrom->GetId())->fSupportsDesiredCmpctVersion) {
5227+
if (pfrom->GetLocalServices() & NODE_WITNESS)
5228+
State(pfrom->GetId())->fSupportsDesiredCmpctVersion = (nCMPCTBLOCKVersion == 2);
5229+
else
5230+
State(pfrom->GetId())->fSupportsDesiredCmpctVersion = (nCMPCTBLOCKVersion == 1);
5231+
}
52045232
}
52055233
}
52065234

@@ -5258,7 +5286,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
52585286
nodestate->nBlocksInFlight < MAX_BLOCKS_IN_TRANSIT_PER_PEER &&
52595287
(!IsWitnessEnabled(chainActive.Tip(), chainparams.GetConsensus()) || State(pfrom->GetId())->fHaveWitness)) {
52605288
inv.type |= nFetchFlags;
5261-
if (nodestate->fProvidesHeaderAndIDs && !(pfrom->GetLocalServices() & NODE_WITNESS))
5289+
if (nodestate->fSupportsDesiredCmpctVersion)
52625290
vToFetch.push_back(CInv(MSG_CMPCT_BLOCK, inv.hash));
52635291
else
52645292
vToFetch.push_back(inv);
@@ -5386,7 +5414,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
53865414
}
53875415
resp.txn[i] = block.vtx[req.indexes[i]];
53885416
}
5389-
pfrom->PushMessageWithFlag(SERIALIZE_TRANSACTION_NO_WITNESS, NetMsgType::BLOCKTXN, resp);
5417+
pfrom->PushMessageWithFlag(State(pfrom->GetId())->fWantsCmpctWitness ? 0 : SERIALIZE_TRANSACTION_NO_WITNESS, NetMsgType::BLOCKTXN, resp);
53905418
}
53915419

53925420

@@ -5650,7 +5678,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
56505678
// We requested this block for some reason, but our mempool will probably be useless
56515679
// so we just grab the block via normal getdata
56525680
std::vector<CInv> vInv(1);
5653-
vInv[0] = CInv(MSG_BLOCK, cmpctblock.header.GetHash());
5681+
vInv[0] = CInv(MSG_BLOCK | GetFetchFlags(pfrom, pindex->pprev, chainparams.GetConsensus()), cmpctblock.header.GetHash());
56545682
pfrom->PushMessage(NetMsgType::GETDATA, vInv);
56555683
}
56565684
return true;
@@ -5662,6 +5690,12 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
56625690

56635691
CNodeState *nodestate = State(pfrom->GetId());
56645692

5693+
if (IsWitnessEnabled(pindex->pprev, chainparams.GetConsensus()) && !nodestate->fSupportsDesiredCmpctVersion) {
5694+
// Don't bother trying to process compact blocks from v1 peers
5695+
// after segwit activates.
5696+
return true;
5697+
}
5698+
56655699
// We want to be a bit conservative just to be extra careful about DoS
56665700
// possibilities in compact block processing...
56675701
if (pindex->nHeight <= chainActive.Height() + 2) {
@@ -5688,7 +5722,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
56885722
} else if (status == READ_STATUS_FAILED) {
56895723
// Duplicate txindexes, the block is now in-flight, so just request it
56905724
std::vector<CInv> vInv(1);
5691-
vInv[0] = CInv(MSG_BLOCK, cmpctblock.header.GetHash());
5725+
vInv[0] = CInv(MSG_BLOCK | GetFetchFlags(pfrom, pindex->pprev, chainparams.GetConsensus()), cmpctblock.header.GetHash());
56925726
pfrom->PushMessage(NetMsgType::GETDATA, vInv);
56935727
return true;
56945728
}
@@ -5715,7 +5749,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
57155749
// We requested this block, but its far into the future, so our
57165750
// mempool will probably be useless - request the block normally
57175751
std::vector<CInv> vInv(1);
5718-
vInv[0] = CInv(MSG_BLOCK, cmpctblock.header.GetHash());
5752+
vInv[0] = CInv(MSG_BLOCK | GetFetchFlags(pfrom, pindex->pprev, chainparams.GetConsensus()), cmpctblock.header.GetHash());
57195753
pfrom->PushMessage(NetMsgType::GETDATA, vInv);
57205754
return true;
57215755
} else {
@@ -5757,7 +5791,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
57575791
} else if (status == READ_STATUS_FAILED) {
57585792
// Might have collided, fall back to getdata now :(
57595793
std::vector<CInv> invs;
5760-
invs.push_back(CInv(MSG_BLOCK, resp.blockhash));
5794+
invs.push_back(CInv(MSG_BLOCK | GetFetchFlags(pfrom, chainActive.Tip(), chainparams.GetConsensus()), resp.blockhash));
57615795
pfrom->PushMessage(NetMsgType::GETDATA, invs);
57625796
} else {
57635797
CValidationState state;
@@ -5906,7 +5940,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
59065940
pindexLast->GetBlockHash().ToString(), pindexLast->nHeight);
59075941
}
59085942
if (vGetData.size() > 0) {
5909-
if (nodestate->fProvidesHeaderAndIDs && vGetData.size() == 1 && mapBlocksInFlight.size() == 1 && pindexLast->pprev->IsValid(BLOCK_VALID_CHAIN) && !(pfrom->GetLocalServices() & NODE_WITNESS)) {
5943+
if (nodestate->fSupportsDesiredCmpctVersion && vGetData.size() == 1 && mapBlocksInFlight.size() == 1 && pindexLast->pprev->IsValid(BLOCK_VALID_CHAIN)) {
59105944
// We seem to be rather well-synced, so it appears pfrom was the first to provide us
59115945
// with this block! Let's get them to announce using compact blocks in the future.
59125946
MaybeSetPeerAsAnnouncingHeaderAndIDs(nodestate, pfrom, connman);
@@ -6536,8 +6570,8 @@ bool SendMessages(CNode* pto, CConnman& connman)
65366570
//TODO: Shouldn't need to reload block from disk, but requires refactor
65376571
CBlock block;
65386572
assert(ReadBlockFromDisk(block, pBestIndex, consensusParams));
6539-
CBlockHeaderAndShortTxIDs cmpctblock(block);
6540-
pto->PushMessageWithFlag(SERIALIZE_TRANSACTION_NO_WITNESS, NetMsgType::CMPCTBLOCK, cmpctblock);
6573+
CBlockHeaderAndShortTxIDs cmpctblock(block, state.fWantsCmpctWitness);
6574+
pto->PushMessageWithFlag(state.fWantsCmpctWitness ? 0 : SERIALIZE_TRANSACTION_NO_WITNESS, NetMsgType::CMPCTBLOCK, cmpctblock);
65416575
state.pindexBestHeaderSent = pBestIndex;
65426576
} else if (state.fPreferHeaders) {
65436577
if (vHeaders.size() > 1) {

‎src/test/blockencodings_tests.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ BOOST_AUTO_TEST_CASE(SimpleRoundTripTest)
6464

6565
// Do a simple ShortTxIDs RT
6666
{
67-
CBlockHeaderAndShortTxIDs shortIDs(block);
67+
CBlockHeaderAndShortTxIDs shortIDs(block, true);
6868

6969
CDataStream stream(SER_NETWORK, PROTOCOL_VERSION);
7070
stream << shortIDs;
@@ -116,7 +116,7 @@ class TestHeaderAndShortIDs {
116116
stream >> *this;
117117
}
118118
TestHeaderAndShortIDs(const CBlock& block) :
119-
TestHeaderAndShortIDs(CBlockHeaderAndShortTxIDs(block)) {}
119+
TestHeaderAndShortIDs(CBlockHeaderAndShortTxIDs(block, true)) {}
120120

121121
uint64_t GetShortID(const uint256& txhash) const {
122122
CDataStream stream(SER_NETWORK, PROTOCOL_VERSION);
@@ -267,7 +267,7 @@ BOOST_AUTO_TEST_CASE(EmptyBlockRoundTripTest)
267267

268268
// Test simple header round-trip with only coinbase
269269
{
270-
CBlockHeaderAndShortTxIDs shortIDs(block);
270+
CBlockHeaderAndShortTxIDs shortIDs(block, false);
271271

272272
CDataStream stream(SER_NETWORK, PROTOCOL_VERSION);
273273
stream << shortIDs;

‎src/txmempool.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -444,7 +444,7 @@ bool CTxMemPool::addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry,
444444
totalTxSize += entry.GetTxSize();
445445
minerPolicyEstimator->processTransaction(entry, fCurrentEstimate);
446446

447-
vTxHashes.emplace_back(hash, newit);
447+
vTxHashes.emplace_back(tx.GetWitnessHash(), newit);
448448
newit->vTxHashesIdx = vTxHashes.size() - 1;
449449

450450
return true;

‎src/txmempool.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -465,7 +465,7 @@ class CTxMemPool
465465
indexed_transaction_set mapTx;
466466

467467
typedef indexed_transaction_set::nth_index<0>::type::iterator txiter;
468-
std::vector<std::pair<uint256, txiter> > vTxHashes; //!< All tx hashes/entries in mapTx, in random order
468+
std::vector<std::pair<uint256, txiter> > vTxHashes; //!< All tx witness hashes/entries in mapTx, in random order
469469

470470
struct CompareIteratorByHash {
471471
bool operator()(const txiter &a, const txiter &b) const {

0 commit comments

Comments
 (0)