Recent Bitcoin code reading efforts led naturally to another round of editing, in a one-two ratchet towards the object of full ownership of the codebase, with each pivot providing new support for the next.
Since I put in the effort to structure the changes into relatively coherent and individually digestible steps, the result is a whopping seven-patch series. While it doesn't solve the lazy block download problem that originally motivated the work, it removes many of the noted hazards that stood in the way. Most significantly, the sending buffer flood control mechanism actually works now, closing a remotely exploitable denial-of-service vulnerability (via 32-bit integer overflow) that's been public and otherwise unresolved for over two years.(i)
Beyond this, the minor wins include, among other things the alert reader may pick up on:
- Effectively limiting the
getheadersresponse so it doesn't trigger a sending flood and disconnect the peer ; as the fix is adopted, it will open up this command as an option for implementing more reliable, efficient and possibly spam-resistant block download. - Removing unneeded I/O and processing in responding to the
getblockscommand, opening up the option of sending it frequently. - Making debug log output for sent and received messages more useful and less flaky.
- Curing the noted warts in error handling and logging.
- Refactoring to eliminate both unused code and repetitive code explosion, achieving an overall ratio of deleted to inserted lines across the series of nearly three to one: specifically, 671 deletions to 251 insertions.
Get the updated release downloader here : fetch-bitcoind-0003.sh, signature ; or read on for details.
In previous code-related articles I've mainly stuck to human-text for the body and linking to external files containing the code under discussion.(ii) This time, due to the significance of the work and perhaps further animated by glimpses of a shiny new code-blogging tool and its rationale, I will again present the bulk of the changes directly for fine-grained elucidation. After all, if the coding was the bulk of the work here then why not let it take center stage?
For a first taste, here's the tidy new log output from a node under attack:
05/09/22 07:09:43 accepted connection 10.9.0.2:46888 05/09/22 07:09:43 recv 10.9.0.2:46888 version (92 B) 05/09/22 07:09:43 send 10.9.0.2:46888 version (115 B) 05/09/22 07:09:43 send 10.9.0.2:46888 verack (0 B) 05/09/22 07:09:43 send 10.9.0.2:46888 getblocks (1029 B) 05/09/22 07:09:43 version message: version 99999, blocks=0 05/09/22 07:09:43 recv 10.9.0.2:46888 verack (0 B) 05/09/22 07:09:43 recv 10.9.0.2:46888 getdata (36003 B) 05/09/22 07:09:43 received getdata for: block 000000000000000000070fd3e03a33256853e78e707a34bc147db08e5ed3adb8 05/09/22 07:09:43 send 10.9.0.2:46888 block (939322 B) 05/09/22 07:09:43 received getdata for: block 000000000000000000070fd3e03a33256853e78e707a34bc147db08e5ed3adb8 05/09/22 07:09:43 send 10.9.0.2:46888 block (939322 B) 05/09/22 07:09:43 received getdata for: block 000000000000000000070fd3e03a33256853e78e707a34bc147db08e5ed3adb8 05/09/22 07:09:43 send 10.9.0.2:46888 block (939322 B) 05/09/22 07:09:43 received getdata for: block 000000000000000000070fd3e03a33256853e78e707a34bc147db08e5ed3adb8 05/09/22 07:09:43 send 10.9.0.2:46888 block (939322 B) 05/09/22 07:09:43 received getdata for: block 000000000000000000070fd3e03a33256853e78e707a34bc147db08e5ed3adb8 05/09/22 07:09:43 send 10.9.0.2:46888 block (939322 B) 05/09/22 07:09:43 received getdata for: block 000000000000000000070fd3e03a33256853e78e707a34bc147db08e5ed3adb8 05/09/22 07:09:43 send 10.9.0.2:46888 block (939322 B) 05/09/22 07:09:43 received getdata for: block 000000000000000000070fd3e03a33256853e78e707a34bc147db08e5ed3adb8 05/09/22 07:09:43 send 10.9.0.2:46888 block (939322 B) 05/09/22 07:09:43 received getdata for: block 000000000000000000070fd3e03a33256853e78e707a34bc147db08e5ed3adb8 05/09/22 07:09:43 send 10.9.0.2:46888 block (939322 B) 05/09/22 07:09:43 received getdata for: block 000000000000000000070fd3e03a33256853e78e707a34bc147db08e5ed3adb8 05/09/22 07:09:43 send 10.9.0.2:46888 block (939322 B) 05/09/22 07:09:43 received getdata for: block 000000000000000000070fd3e03a33256853e78e707a34bc147db08e5ed3adb8 05/09/22 07:09:43 send 10.9.0.2:46888 block (939322 B) 05/09/22 07:09:43 received getdata for: block 000000000000000000070fd3e03a33256853e78e707a34bc147db08e5ed3adb8 05/09/22 07:09:43 send 10.9.0.2:46888 block (939322 B) 05/09/22 07:09:43 socket send flood control disconnect (10004110 B) 05/09/22 07:09:43 disconnecting node 10.9.0.2:46888 05/09/22 07:09:43 ProcessMessages() : peer disconnected
Table of Patches
- bitcoin_response_size_limits_1.vpatch
- bitcoin_response_size_limits_2.vpatch
- bitcoin_pushmessage_cleanup_1.vpatch
- bitcoin_pushmessage_cleanup_2.vpatch
- bitcoin_enforce_buffer_limits.vpatch
- bitcoin_posix_error_handling.vpatch
- bitcoin_rebranding.vpatch
bitcoin_response_size_limits_1.vpatch
(top - view patch - browse tree)
Description (from the manifest): Simple adjustments to prepare the code for more significant ones to improve response size limiting at the source.
- CBlockLocator::GetBlockIndex never returns NULL unless something is very wrong.
- Don't log when the getblocks response stops at the requested hash: it's uninteresting since that hash was already logged just before, and this allows simplifying loop bounds.
- Differentiate getheaders implementation between the null locator (single block) case and the normal loop.
- Always log starting heights for getblocks/getheaders rather than punting to -1 just because a non-null pointer wasn't immediately available.
diff -uNr a/bitcoin/src/main.cpp b/bitcoin/src/main.cpp
--- a/bitcoin/src/main.cpp a30a8ae3e233c6f2b132532b85cedd41dc75defb30309abc5a58cd6d7c9c50b0880c88a6ba7b3440d590e21e29b0b90901b2959ac12c8558bb6ad50ab4d995e6
+++ b/bitcoin/src/main.cpp ea612c8910efd8e86d1a9eaa3668de9c8d586d495db3567b8e4115a070c3c6e8f07e60797411f0b511e4290877fa828c181bb730093812cc05f9db4620868345
@@ -1958,18 +1958,18 @@
CBlockIndex* pindex = locator.GetBlockIndex();
// Send the rest of the chain
In the "getblocks" message handler, we eliminate a bogus "-1" starting height printed to the log by capturing the height value prior to advancing the block index pointer, i.e. while it's still guaranteed non-null.
- if (pindex)
- pindex = pindex->pnext;
+ if (!pindex)
+ {
+ printf("getblocks: null locator block index (shouldn't happen!)\n");
+ return false;
+ }
+ unsigned int nStartHeight = pindex->nHeight + 1;
+ pindex = pindex->pnext;
int nLimit = 500 + locator.GetDistanceBack();
unsigned int nBytes = 0;
- printf("getblocks %d to %s limit %d\n", (pindex ? pindex->nHeight : -1), hashStop.ToString().c_str(), nLimit);
- for (; pindex; pindex = pindex->pnext)
+ printf("getblocks %d to %s limit %d\n", nStartHeight, hashStop.ToString().c_str(), nLimit);
+ for (; pindex && pindex->GetBlockHash() != hashStop; pindex = pindex->pnext)
{
- if (pindex->GetBlockHash() == hashStop)
- {
- printf(" getblocks stopping at %d %s (%u bytes)\n", pindex->nHeight, pindex->GetBlockHash().ToString().c_str(), nBytes);
- break;
- }
pfrom->PushInventory(CInv(MSG_BLOCK, pindex->GetBlockHash()));
CBlock block;
block.ReadFromDisk(pindex, true);
Next is a similar transformation for the "getheaders" handler. As with "getblocks", this command receives a block locator object to negotiate a shared starting point and an optional block hash as the stopping point (optional in that it's sometimes sent as all-zeros which won't match any real block). However, this time there's a special case that if the locator is null, it's interpreted as a request for the header of the exact block given by the stopping hash. I chose to reduce the code sharing between these two cases; although it makes for (ever so slightly) more code, it reduces the mental overhead of considering further changes in the normal (multi-header) case. Also it allows specialization of the log message: reporting a "limit" makes little sense when it's exactly one block being requested, and perhaps it'll be nice to have a way to see if the special case is ever even used in practice.
@@ -1993,6 +1993,7 @@
vRecv >> locator >> hashStop;
CBlockIndex* pindex = NULL;
+ vector<CBlock> vHeaders;
if (locator.IsNull())
{
// If locator is null, return the hashStop block
@@ -2000,23 +2001,28 @@
if (mi == mapBlockIndex.end())
return true;
pindex = (*mi).second;
+ printf("getheaders %d at %s\n", pindex->nHeight, hashStop.ToString().c_str());
+ vHeaders.push_back(pindex->GetBlockHeader());
}
else
{
// Find the last block the caller has in the main chain
pindex = locator.GetBlockIndex();
- if (pindex)
- pindex = pindex->pnext;
- }
-
- vector<CBlock> vHeaders;
- int nLimit = 2000 + locator.GetDistanceBack();
- printf("getheaders %d to %s limit %d\n", (pindex ? pindex->nHeight : -1), hashStop.ToString().c_str(), nLimit);
- for (; pindex; pindex = pindex->pnext)
- {
- vHeaders.push_back(pindex->GetBlockHeader());
- if (--nLimit <= 0 || pindex->GetBlockHash() == hashStop)
- break;
This may be one of the more difficult sections to follow due to the change of indentation, but note that the content of most lines is unchanged; all that's really happening is that the vHeaders declaration moved upward and nStartHeight is introduced for accurate logging exactly as before.
+ if (!pindex)
+ {
+ printf("getheaders: null locator block index (shouldn't happen!)\n");
+ return false;
+ }
+ unsigned int nStartHeight = pindex->nHeight + 1;
+ pindex = pindex->pnext;
+ int nLimit = 2000 + locator.GetDistanceBack();
+ printf("getheaders %d to %s limit %d\n", nStartHeight, hashStop.ToString().c_str(), nLimit);
+ for (; pindex; pindex = pindex->pnext)
+ {
+ vHeaders.push_back(pindex->GetBlockHeader());
+ if (--nLimit <= 0 || pindex->GetBlockHash() == hashStop)
+ break;
+ }
}
pfrom->PushMessage("headers", vHeaders);
}
bitcoin_response_size_limits_2.vpatch
(top - view patch - browse tree)
Now we'll be looking at the same two sections of code and replacing the various weird block count limits with much simpler ones. Since a negative limit makes no sense, the integer type of nLimit is corrected to unsigned. First, for getblocks:
diff -uNr a/bitcoin/src/main.cpp b/bitcoin/src/main.cpp
--- a/bitcoin/src/main.cpp ea612c8910efd8e86d1a9eaa3668de9c8d586d495db3567b8e4115a070c3c6e8f07e60797411f0b511e4290877fa828c181bb730093812cc05f9db4620868345
+++ b/bitcoin/src/main.cpp 00dc0778ec8886372de6a2f33a914f4e5d0360d7bb690b242f770f259ac562bfd3f663c9bd554f911a0b935522ee1377ffd0eaef0c65d525ae17ebd60a9a23ed
@@ -1965,20 +1965,19 @@
}
unsigned int nStartHeight = pindex->nHeight + 1;
pindex = pindex->pnext;
- int nLimit = 500 + locator.GetDistanceBack();
- unsigned int nBytes = 0;
+ // Set a conservative limit to avoid flooding the sending buffer and ensure hashContinue is reached when processing the anticipated followup getdata, yet keeping the processing and I/O for this response low. Think of it as a pipeline depth for the download process.
+ unsigned int nLimit = SendBufferSize() / MAX_BLOCK_SIZE / 2;
+ if (nLimit == 0) nLimit = 1;
printf("getblocks %d to %s limit %d\n", nStartHeight, hashStop.ToString().c_str(), nLimit);
for (; pindex && pindex->GetBlockHash() != hashStop; pindex = pindex->pnext)
{
pfrom->PushInventory(CInv(MSG_BLOCK, pindex->GetBlockHash()));
- CBlock block;
- block.ReadFromDisk(pindex, true);
- nBytes += block.GetSerializeSize(SER_NETWORK);
- if (--nLimit <= 0 || nBytes >= SendBufferSize()/2)
+ if (--nLimit == 0)
{
// When this block is requested, we'll send an inv that'll make them
// getblocks the next batch of inventory.
- printf(" getblocks stopping at limit %d %s (%u bytes)\n", pindex->nHeight, pindex->GetBlockHash().ToString().c_str(), nBytes);
+ printf(" getblocks stopping at limit %d %s\n", pindex->nHeight, pindex->GetBlockHash().ToString().c_str());
Since we're no longer loading blocks from disk and serializing just to measure their size, we can't log the accumulated size of the anticipated batch anymore. No big loss there that I can see.
Note that this results in a fixed batch size (5 blocks at the default 10M sending buffer size), so batches will be much smaller than they "could" be for the early history where blocks were much smaller than the limit. Again I don't see a problem; it shouldn't slow things down much and even if it does, those early blocks don't amount to a significant portion of the overall sync time anyway.
+ // TODO somehow move this state tracking to the recipient side because there's no guarantee that it sends a getdata for the continuation block.
pfrom->hashContinue = pindex->GetBlockHash();
break;
}
And for getheaders. We measure the serialized size of one particular header and assume they're all roughly the same. (I expect they're exactly the same, but conceivably there could be variable-width integer encodings or the like involved. Most of a header's weight though is in hashes which are definitely fixed.)
@@ -2014,13 +2013,15 @@
return false;
}
unsigned int nStartHeight = pindex->nHeight + 1;
+ // Avoid flooding the sending buffer, with room to spare for overhead and other requests that may be pending.
+ unsigned int nLimit = SendBufferSize() / pindex->GetBlockHeader().GetSerializeSize() / 2;
+ if (nLimit == 0) nLimit = 1;
pindex = pindex->pnext;
- int nLimit = 2000 + locator.GetDistanceBack();
printf("getheaders %d to %s limit %d\n", nStartHeight, hashStop.ToString().c_str(), nLimit);
for (; pindex; pindex = pindex->pnext)
{
vHeaders.push_back(pindex->GetBlockHeader());
- if (--nLimit <= 0 || pindex->GetBlockHash() == hashStop)
+ if (--nLimit == 0 || pindex->GetBlockHash() == hashStop)
break;
}
}
bitcoin_pushmessage_cleanup_1.vpatch
(top - view patch - browse tree)
Description:
- Tame the PushMessage template explosion by pushing CDataStream serialization out to callers.
- Permanently disable undocumented -allowreceivebyip and otherwise dead code turned up when auditing PushMessage callers, namely generic network request tracking and publish/subscribe infrastructure.
- Simplify initialization of vSend/vRecv fields: type (since SER_NETWORK is the default) and version (since the indicated flag day is long past; it wasn't quite clear to remove the field altogether).
- Add unit test for assumption noted in one of the PushMessage calls.
diff -uNr a/bitcoin/src/main.cpp b/bitcoin/src/main.cpp
--- a/bitcoin/src/main.cpp 00dc0778ec8886372de6a2f33a914f4e5d0360d7bb690b242f770f259ac562bfd3f663c9bd554f911a0b935522ee1377ffd0eaef0c65d525ae17ebd60a9a23ed
+++ b/bitcoin/src/main.cpp 5b796065e6bdfce53478af230763776d8342f0da923744cc8f76b9802ad8d01f6bdde0149acc784491130d3033662c9d1602bde39ec213c958cd788cdc3ffef6
@@ -1679,7 +1679,6 @@
bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv)
{
- static map<unsigned int, vector<unsigned char> > mapReuseKey;
RandAddSeedPerfmon();
if (fDebug) {
printf("%s ", DateTimeStrFormat("%x %H:%M:%S", GetTime()).c_str());
@@ -1911,7 +1910,7 @@
{
CBlock block;
block.ReadFromDisk((*mi).second);
- pfrom->PushMessage("block", block);
+ pfrom->PushMessage("block", pfrom->SendingStream() << block);
We'll be making this change to most of the PushMessage calls. If you're inclined to object that it's poor form to push this kind of stream-construction boilerplate out to the caller since they're all doing the exact same thing, consider witholding judgement until you see the river of blood removed by this device.
// Trigger them to send a getblocks request for the next batch of inventory
if (inv.hash == pfrom->hashContinue)
@@ -1921,7 +1920,7 @@
// wait for other stuff first.
vector<CInv> vInv;
vInv.push_back(CInv(MSG_BLOCK, hashBestChain));
- pfrom->PushMessage("inv", vInv);
+ pfrom->PushMessage("inv", pfrom->SendingStream() << vInv);
pfrom->hashContinue = 0;
}
}
@@ -1933,6 +1932,7 @@
{
map<CInv, CDataStream>::iterator mi = mapRelay.find(inv);
if (mi != mapRelay.end())
+ // It's OK to pass the existing CDataStream directly. With the prior family of PushMessage templates it got re-serialized (<<) into vSend while now it's appended (+=), but these are equivalent as per the "Special case" in serialize.h.
pfrom->PushMessage(inv.GetCommand(), (*mi).second);
}
}
@@ -2025,7 +2025,7 @@
break;
}
}
- pfrom->PushMessage("headers", vHeaders);
+ pfrom->PushMessage("headers", pfrom->SendingStream() << vHeaders);
}
For the context that didn't quite make it in, below is the "checkorder" message handler. The idea seemed to be for pushing out an "order" of some sort to the network to negotiate a payment address, one of the many "throwing shit at the wall and seeing what sticks" features of the early Bitcoin implementation (and virtually every present-day altcoin implementation).
We preserve its disabled stub case for compatibility. As that's the only thing that generates a "reply" message and we're certainly not initiating any such exchange, we can remove the "reply" handler altogether, and with it all the "request tracker" cruft.
Although, my "fiendishly clever" streak notes that this is perhaps the only request-response pair in the whole protocol allowing for a unique or randomized identifier (e.g. even "ping" doesn't send any response) and as such could be reused as a synchronization or timing mechanism. Hypothetically speaking, of course.
@@ -2101,45 +2101,8 @@
uint256 hashReply;
vRecv >> hashReply;
- if (!GetBoolArg("-allowreceivebyip"))
- {
- pfrom->PushMessage("reply", hashReply, (int)2, string(""));
- return true;
- }
-
- CWalletTx order;
- vRecv >> order;
-
- /// we have a chance to check the order here
-
- // Keep giving the same key to the same ip until they use it
- if (!mapReuseKey.count(pfrom->addr.ip))
- pwalletMain->GetKeyFromPool(mapReuseKey[pfrom->addr.ip], true);
-
- // Send back approval of order and pubkey to use
- CScript scriptPubKey;
- scriptPubKey << mapReuseKey[pfrom->addr.ip] << OP_CHECKSIG;
- pfrom->PushMessage("reply", hashReply, (int)0, scriptPubKey);
- }
-
-
- else if (strCommand == "reply")
- {
- uint256 hashReply;
- vRecv >> hashReply;
-
- CRequestTracker tracker;
- CRITICAL_BLOCK(pfrom->cs_mapRequests)
- {
- map<uint256, CRequestTracker>::iterator mi = pfrom->mapRequests.find(hashReply);
- if (mi != pfrom->mapRequests.end())
- {
- tracker = (*mi).second;
- pfrom->mapRequests.erase(mi);
- }
- }
- if (!tracker.IsNull())
- tracker.fn(tracker.param1, vRecv);
+ // receive-by-IP feature permanently disabled
+ pfrom->PushMessage("reply", pfrom->SendingStream() << hashReply << (int)2 << string(""));
}
@@ -2367,14 +2330,14 @@
// receiver rejects addr messages larger than 1000
if (vAddr.size() >= 1000)
{
- pto->PushMessage("addr", vAddr);
+ pto->PushMessage("addr", pto->SendingStream() << vAddr);
vAddr.clear();
}
}
}
pto->vAddrToSend.clear();
if (!vAddr.empty())
- pto->PushMessage("addr", vAddr);
+ pto->PushMessage("addr", pto->SendingStream() << vAddr);
}
@@ -2425,7 +2388,7 @@
vInv.push_back(inv);
if (vInv.size() >= 1000)
{
- pto->PushMessage("inv", vInv);
+ pto->PushMessage("inv", pto->SendingStream() << vInv);
vInv.clear();
}
}
@@ -2433,7 +2396,7 @@
pto->vInventoryToSend = vInvWait;
}
if (!vInv.empty())
- pto->PushMessage("inv", vInv);
+ pto->PushMessage("inv", pto->SendingStream() << vInv);
//
@@ -2451,7 +2414,7 @@
vGetData.push_back(inv);
if (vGetData.size() >= 1000)
{
- pto->PushMessage("getdata", vGetData);
+ pto->PushMessage("getdata", pto->SendingStream() << vGetData);
vGetData.clear();
}
}
@@ -2459,7 +2422,7 @@
pto->mapAskFor.erase(pto->mapAskFor.begin());
}
if (!vGetData.empty())
- pto->PushMessage("getdata", vGetData);
+ pto->PushMessage("getdata", pto->SendingStream() << vGetData);
}
return true;
diff -uNr a/bitcoin/src/main.h b/bitcoin/src/main.h
--- a/bitcoin/src/main.h e1aa413b33f7ec0c3943aae831a5a2fc3795360707d44a31ebb5f810300d8b17ca1dbfd5ddc7785f02366f857694d0eb2746aa27efdd2d40b220387d2baa88ca
+++ b/bitcoin/src/main.h d70c7dd93b2bea85ffe52d440b60d629c4cf125d9ae91afef3aa6c854981c19070663d3a8839455b700e410435634f59fb3ccaf7642595e4374c4bb323261267
@@ -23,7 +23,6 @@
class CAddress;
class CInv;
-class CRequestTracker;
class CNode;
class CBlockIndex;
diff -uNr a/bitcoin/src/net.cpp b/bitcoin/src/net.cpp
--- a/bitcoin/src/net.cpp 6d7c7634cce09792942fc69cf945b64e8f8e77b496ad6bbaed12dccbc5e13c31fc2f1162735cae7951893fb6b3635955703059b1e8ba1607cc483c494c0a126c
+++ b/bitcoin/src/net.cpp b5ef3c960b24f41ac4cac25eef4dd17f6da3b10d7d404b8048dd30519e994908b11c64f94115cd55a732164c15c4b62ff5f454fe771a5843a7474f239d6f87ee
@@ -59,7 +59,7 @@
void CNode::PushGetBlocks(CBlockIndex* pindexBegin, uint256 hashEnd)
{
- PushMessage("getblocks", CBlockLocator(pindexBegin), hashEnd);
+ PushMessage("getblocks", SendingStream() << CBlockLocator(pindexBegin) << hashEnd);
}
@@ -319,111 +319,6 @@
}
}
-
-
-
-
-void AbandonRequests(void (*fn)(void*, CDataStream&), void* param1)
-{
- // If the dialog might get closed before the reply comes back,
- // call this in the destructor so it doesn't get called after it's deleted.
- CRITICAL_BLOCK(cs_vNodes)
- {
- BOOST_FOREACH(CNode* pnode, vNodes)
- {
- CRITICAL_BLOCK(pnode->cs_mapRequests)
- {
- for (map<uint256, CRequestTracker>::iterator mi = pnode->mapRequests.begin(); mi != pnode->mapRequests.end();)
- {
- CRequestTracker& tracker = (*mi).second;
- if (tracker.fn == fn && tracker.param1 == param1)
- pnode->mapRequests.erase(mi++);
- else
- mi++;
- }
- }
- }
- }
-}
-
-
-
-
-
-
-
-//
-// Subscription methods for the broadcast and subscription system.
-// Channel numbers are message numbers, i.e. MSG_TABLE and MSG_PRODUCT.
-//
-// The subscription system uses a meet-in-the-middle strategy.
-// With 100,000 nodes, if senders broadcast to 1000 random nodes and receivers
-// subscribe to 1000 random nodes, 99.995% (1 - 0.99^1000) of messages will get through.
-//
-
-bool AnySubscribed(unsigned int nChannel)
-{
- if (pnodeLocalHost->IsSubscribed(nChannel))
- return true;
- CRITICAL_BLOCK(cs_vNodes)
- BOOST_FOREACH(CNode* pnode, vNodes)
- if (pnode->IsSubscribed(nChannel))
- return true;
- return false;
-}
-
-bool CNode::IsSubscribed(unsigned int nChannel)
-{
- if (nChannel >= vfSubscribe.size())
- return false;
- return vfSubscribe[nChannel];
-}
-
-void CNode::Subscribe(unsigned int nChannel, unsigned int nHops)
-{
- if (nChannel >= vfSubscribe.size())
- return;
-
- if (!AnySubscribed(nChannel))
- {
- // Relay subscribe
- CRITICAL_BLOCK(cs_vNodes)
- BOOST_FOREACH(CNode* pnode, vNodes)
- if (pnode != this)
- pnode->PushMessage("subscribe", nChannel, nHops);
- }
-
- vfSubscribe[nChannel] = true;
-}
-
-void CNode::CancelSubscribe(unsigned int nChannel)
-{
- if (nChannel >= vfSubscribe.size())
- return;
-
- // Prevent from relaying cancel if wasn't subscribed
- if (!vfSubscribe[nChannel])
- return;
- vfSubscribe[nChannel] = false;
-
- if (!AnySubscribed(nChannel))
- {
- // Relay subscription cancel
- CRITICAL_BLOCK(cs_vNodes)
- BOOST_FOREACH(CNode* pnode, vNodes)
- if (pnode != this)
- pnode->PushMessage("sub-cancel", nChannel);
- }
-}
-
-
-
-
-
-
-
-
-
CNode* FindNode(unsigned int ip)
{
CRITICAL_BLOCK(cs_vNodes)
@@ -513,18 +408,6 @@
}
}
-void CNode::Cleanup()
-{
- // All of a nodes broadcasts and subscriptions are automatically torn down
- // when it goes down, so a node has to stay up to keep its broadcast going.
-
- // Cancel subscriptions
- for (unsigned int nChannel = 0; nChannel < vfSubscribe.size(); nChannel++)
- if (vfSubscribe[nChannel])
- CancelSubscribe(nChannel);
-}
-
-
std::map<unsigned int, int64> CNode::setBanned;
CCriticalSection CNode::cs_setBanned;
@@ -626,7 +509,6 @@
// close socket and cleanup
pnode->CloseSocketDisconnect();
- pnode->Cleanup();
// hold in disconnected pool until all refs are released
pnode->nReleaseTime = max(pnode->nReleaseTime, GetTime() + 15 * 60);
@@ -646,7 +528,6 @@
bool fDelete = false;
TRY_CRITICAL_BLOCK(pnode->cs_vSend)
TRY_CRITICAL_BLOCK(pnode->cs_vRecv)
- TRY_CRITICAL_BLOCK(pnode->cs_mapRequests)
TRY_CRITICAL_BLOCK(pnode->cs_inventory)
fDelete = true;
if (fDelete)
diff -uNr a/bitcoin/src/net.h b/bitcoin/src/net.h
--- a/bitcoin/src/net.h 492c9cc92a504bb8174d75fafcbee6980986182a459efc9bfa1d64766320d98ba2fa971d78d00a777c6cc50f82a5d424997927378e99738b1b3b550bdaa727f7
+++ b/bitcoin/src/net.h 446bb5f05b776039fd25680c8b46d8aada9447be55d56d882dbf52a5c1d527b02241a2fdf9985836e344128dbcf6fb3f118afac37b80109867cb05856906a598
@@ -13,7 +13,6 @@
#include "protocol.h"
class CAddrDB;
-class CRequestTracker;
class CNode;
class CBlockIndex;
extern int nBestHeight;
@@ -23,7 +22,6 @@
inline unsigned int ReceiveBufferSize() { return 1000*GetArg("-maxreceivebuffer", 10*1000); }
inline unsigned int SendBufferSize() { return 1000*GetArg("-maxsendbuffer", 10*1000); }
-static const unsigned int PUBLISH_HOPS = 5;
bool ConnectSocket(const CAddress& addrConnect, SOCKET& hSocketRet, int nTimeout=nConnectTimeout);
bool Lookup(const char *pszName, std::vector<CAddress>& vaddr, int nServices, int nMaxSolutions, int portDefault = 0, bool fAllowPort = false);
@@ -32,8 +30,6 @@
void AddressCurrentlyConnected(const CAddress& addr);
CNode* FindNode(unsigned int ip);
CNode* ConnectNode(CAddress addrConnect, int64 nTimeout=0);
-void AbandonRequests(void (*fn)(void*, CDataStream&), void* param1);
-bool AnySubscribed(unsigned int nChannel);
void MapPort(bool fMapPort);
bool BindListenPort(std::string& strError=REF(std::string()));
void StartNode(void* parg);
@@ -45,28 +41,6 @@
MSG_BLOCK,
};
-class CRequestTracker
-{
-public:
- void (*fn)(void*, CDataStream&);
- void* param1;
-
- explicit CRequestTracker(void (*fnIn)(void*, CDataStream&)=NULL, void* param1In=NULL)
- {
- fn = fnIn;
- param1 = param1In;
- }
-
- bool IsNull()
- {
- return fn == NULL;
- }
-};
-
-
-
-
-
extern bool fClient;
extern bool fAllowDNS;
extern uint64 nLocalServices;
@@ -127,8 +101,6 @@
public:
int64 nReleaseTime;
- std::map<uint256, CRequestTracker> mapRequests;
- CCriticalSection cs_mapRequests;
uint256 hashContinue;
int nStartingHeight;
@@ -144,23 +116,13 @@
CCriticalSection cs_inventory;
std::multimap<int64, CInv> mapAskFor;
- // publish and subscription
- std::vector<char> vfSubscribe;
-
CNode(SOCKET hSocketIn, CAddress addrIn, bool fInboundIn=false)
{
nServices = 0;
hSocket = hSocketIn;
- vSend.SetType(SER_NETWORK);
- vSend.SetVersion(0);
- vRecv.SetType(SER_NETWORK);
- vRecv.SetVersion(0);
It's difficult to track down all the implications of this versioning (for one thing, it affects serialization and can be changed later by the peer), but at least it's easy to see that the below test is now always true, at least unless one's clock is seriously off.
// Version 0.2 obsoletes 20 Feb 2012
- if (GetTime() > 1329696000)
- {
- vSend.SetVersion(209);
- vRecv.SetVersion(209);
- }
+ vSend.SetVersion(209);
+ vRecv.SetVersion(209);
nLastSend = 0;
nLastRecv = 0;
nLastSendEmpty = GetTime();
@@ -180,7 +142,6 @@
hashContinue = 0;
nStartingHeight = -1;
fGetAddr = false;
- vfSubscribe.assign(256, false);
nMisbehavior = 0;
// Be shy and don't send version until we hear
@@ -271,7 +232,11 @@
mapAskFor.insert(std::make_pair(nRequestTime, inv));
}
-
+ // Construct a compatible output stream for passing to PushMessage
+ CDataStream SendingStream()
+ {
+ return CDataStream(SER_NETWORK, vSend.GetVersion());
+ }
void BeginMessage(const char* pszCommand)
{
Before noticing the full horrors of the serialization-versioning business, I tried to do the PushMessage calls using the global CDataStream() constructor rather than having to add the above member function as a wrapper-constructor or "factory". The result was that the peers failed to negotiate their protocol version properly and further commands didn't make it through (reporting "ProcessMessage FAILED" to the log).
Oh yes, this series of revisions has itself been through a series of revisions. To manage it all, I used an "exploded" structure with multiple copies of the tree named according to the patches, with numbered symlinks to keep track of the order and a/b symlinks adjusted for whichever I needed to compare. For testing, I'd copy just the "src" subtree over to a temporary build tree where "boost" and its fat friends were already built and ready to go.
@@ -355,8 +320,15 @@
CAddress addrYou = (fUseProxy ? CAddress("0.0.0.0") : addr);
CAddress addrMe = (fUseProxy || !addrLocalHost.IsRoutable() ? CAddress("0.0.0.0") : addrLocalHost);
RAND_bytes((unsigned char*)&nLocalHostNonce, sizeof(nLocalHostNonce));
- PushMessage("version", VERSION, nLocalServices, nTime, addrYou, addrMe,
- nLocalHostNonce, FormatSubVersion(CLIENT_NAME, VERSION), nBestHeight);
+ PushMessage("version", SendingStream()
+ << VERSION
+ << nLocalServices
+ << nTime
+ << addrYou
+ << addrMe
+ << nLocalHostNonce
+ << FormatSubVersion(CLIENT_NAME, VERSION)
+ << nBestHeight);
}
Below, the promised river of blood. Ten overloaded nearly-identical variants of PushMessage are reduced to two, the first of which merely a convenience wrapper to avoid downstream changes in the no-payload case. Otherwise, it now takes a pre-serialized payload in the form of a CDataStream, which it appends to the sending buffer in place of serializing objects directly into said buffer. We still haven't cured the overflow, but have reduced the intake side to ONE function.
So much for the famed "template metaprogramming" of C++. It wishes it were like macros in Lisp, yet apparently fails to provide for such a basic thing as a function with variable argument count. But hey, maybe they'll fix it in C++0x...(iii)
@@ -364,41 +336,15 @@
void PushMessage(const char* pszCommand)
{
- try
- {
- BeginMessage(pszCommand);
- EndMessage();
- }
- catch (...)
- {
- AbortMessage();
- throw;
- }
- }
-
- template<typename T1>
- void PushMessage(const char* pszCommand, const T1& a1)
- {
- try
- {
- BeginMessage(pszCommand);
- vSend << a1;
- EndMessage();
- }
- catch (...)
- {
- AbortMessage();
- throw;
- }
+ PushMessage(pszCommand, SendingStream());
}
- template<typename T1, typename T2>
- void PushMessage(const char* pszCommand, const T1& a1, const T2& a2)
+ void PushMessage(const char* pszCommand, const CDataStream& payload)
{
try
{
BeginMessage(pszCommand);
- vSend << a1 << a2;
+ vSend += payload;
EndMessage();
}
catch (...)
@@ -408,165 +354,8 @@
}
}
- template<typename T1, typename T2, typename T3>
- void PushMessage(const char* pszCommand, const T1& a1, const T2& a2, const T3& a3)
- {
- try
- {
- BeginMessage(pszCommand);
- vSend << a1 << a2 << a3;
- EndMessage();
- }
- catch (...)
- {
- AbortMessage();
- throw;
- }
- }
-
- template<typename T1, typename T2, typename T3, typename T4>
- void PushMessage(const char* pszCommand, const T1& a1, const T2& a2, const T3& a3, const T4& a4)
- {
- try
- {
- BeginMessage(pszCommand);
- vSend << a1 << a2 << a3 << a4;
- EndMessage();
- }
- catch (...)
- {
- AbortMessage();
- throw;
- }
- }
-
- template<typename T1, typename T2, typename T3, typename T4, typename T5>
- void PushMessage(const char* pszCommand, const T1& a1, const T2& a2, const T3& a3, const T4& a4, const T5& a5)
- {
- try
- {
- BeginMessage(pszCommand);
- vSend << a1 << a2 << a3 << a4 << a5;
- EndMessage();
- }
- catch (...)
- {
- AbortMessage();
- throw;
- }
- }
-
- template<typename T1, typename T2, typename T3, typename T4, typename T5, typename T6>
- void PushMessage(const char* pszCommand, const T1& a1, const T2& a2, const T3& a3, const T4& a4, const T5& a5, const T6& a6)
- {
- try
- {
- BeginMessage(pszCommand);
- vSend << a1 << a2 << a3 << a4 << a5 << a6;
- EndMessage();
- }
- catch (...)
- {
- AbortMessage();
- throw;
- }
- }
-
- template<typename T1, typename T2, typename T3, typename T4, typename T5, typename T6, typename T7>
- void PushMessage(const char* pszCommand, const T1& a1, const T2& a2, const T3& a3, const T4& a4, const T5& a5, const T6& a6, const T7& a7)
- {
- try
- {
- BeginMessage(pszCommand);
- vSend << a1 << a2 << a3 << a4 << a5 << a6 << a7;
- EndMessage();
- }
- catch (...)
- {
- AbortMessage();
- throw;
- }
- }
-
- template<typename T1, typename T2, typename T3, typename T4, typename T5, typename T6, typename T7, typename T8>
- void PushMessage(const char* pszCommand, const T1& a1, const T2& a2, const T3& a3, const T4& a4, const T5& a5, const T6& a6, const T7& a7, const T8& a8)
- {
- try
- {
- BeginMessage(pszCommand);
- vSend << a1 << a2 << a3 << a4 << a5 << a6 << a7 << a8;
- EndMessage();
- }
- catch (...)
- {
- AbortMessage();
- throw;
- }
- }
-
- template<typename T1, typename T2, typename T3, typename T4, typename T5, typename T6, typename T7, typename T8, typename T9>
- void PushMessage(const char* pszCommand, const T1& a1, const T2& a2, const T3& a3, const T4& a4, const T5& a5, const T6& a6, const T7& a7, const T8& a8, const T9& a9)
- {
- try
- {
- BeginMessage(pszCommand);
- vSend << a1 << a2 << a3 << a4 << a5 << a6 << a7 << a8 << a9;
- EndMessage();
- }
- catch (...)
- {
- AbortMessage();
- throw;
- }
- }
-
-
- void PushRequest(const char* pszCommand,
- void (*fn)(void*, CDataStream&), void* param1)
- {
- uint256 hashReply;
- RAND_bytes((unsigned char*)&hashReply, sizeof(hashReply));
-
- CRITICAL_BLOCK(cs_mapRequests)
- mapRequests[hashReply] = CRequestTracker(fn, param1);
-
- PushMessage(pszCommand, hashReply);
- }
-
- template<typename T1>
- void PushRequest(const char* pszCommand, const T1& a1,
- void (*fn)(void*, CDataStream&), void* param1)
- {
- uint256 hashReply;
- RAND_bytes((unsigned char*)&hashReply, sizeof(hashReply));
-
- CRITICAL_BLOCK(cs_mapRequests)
- mapRequests[hashReply] = CRequestTracker(fn, param1);
-
- PushMessage(pszCommand, hashReply, a1);
- }
-
- template<typename T1, typename T2>
- void PushRequest(const char* pszCommand, const T1& a1, const T2& a2,
- void (*fn)(void*, CDataStream&), void* param1)
- {
- uint256 hashReply;
- RAND_bytes((unsigned char*)&hashReply, sizeof(hashReply));
-
- CRITICAL_BLOCK(cs_mapRequests)
- mapRequests[hashReply] = CRequestTracker(fn, param1);
-
- PushMessage(pszCommand, hashReply, a1, a2);
- }
-
-
-
void PushGetBlocks(CBlockIndex* pindexBegin, uint256 hashEnd);
- bool IsSubscribed(unsigned int nChannel);
- void Subscribe(unsigned int nChannel, unsigned int nHops=0);
- void CancelSubscribe(unsigned int nChannel);
void CloseSocketDisconnect();
- void Cleanup();
// Denial-of-service detection/prevention
@@ -634,59 +423,4 @@
RelayInventory(inv);
}
-
-
-
-
-
-
-
-//
-// Templates for the publish and subscription system.
-// The object being published as T& obj needs to have:
-// a set<unsigned int> setSources member
-// specializations of AdvertInsert and AdvertErase
-// Currently implemented for CTable and CProduct.
-//
-
-template<typename T>
-void AdvertStartPublish(CNode* pfrom, unsigned int nChannel, unsigned int nHops, T& obj)
-{
- // Add to sources
- obj.setSources.insert(pfrom->addr.ip);
-
- if (!AdvertInsert(obj))
- return;
-
- // Relay
- CRITICAL_BLOCK(cs_vNodes)
- BOOST_FOREACH(CNode* pnode, vNodes)
- if (pnode != pfrom && (nHops < PUBLISH_HOPS || pnode->IsSubscribed(nChannel)))
- pnode->PushMessage("publish", nChannel, nHops, obj);
-}
-
-template<typename T>
-void AdvertStopPublish(CNode* pfrom, unsigned int nChannel, unsigned int nHops, T& obj)
-{
- uint256 hash = obj.GetHash();
-
- CRITICAL_BLOCK(cs_vNodes)
- BOOST_FOREACH(CNode* pnode, vNodes)
- if (pnode != pfrom && (nHops < PUBLISH_HOPS || pnode->IsSubscribed(nChannel)))
- pnode->PushMessage("pub-cancel", nChannel, nHops, hash);
-
- AdvertErase(obj);
-}
-
-template<typename T>
-void AdvertRemoveSource(CNode* pfrom, unsigned int nChannel, unsigned int nHops, T& obj)
-{
- // Remove a source
- obj.setSources.erase(pfrom->addr.ip);
-
- // If no longer supported by any sources, cancel it
- if (obj.setSources.empty())
- AdvertStopPublish(pfrom, nChannel, nHops, obj);
-}
-
#endif
diff -uNr a/bitcoin/src/test/serialize_tests.cpp b/bitcoin/src/test/serialize_tests.cpp
--- a/bitcoin/src/test/serialize_tests.cpp false
+++ b/bitcoin/src/test/serialize_tests.cpp 4f0bc192b6224dc1536527732a6de9be37d0675129b57c7abe62d418f0414f73a762fae26c047aeceb39387886480ecf8687b888a56648adbae1b4196138fe4a
@@ -0,0 +1,19 @@
+#include <boost/test/unit_test.hpp>
+
+#include "../serialize.h"
+
+BOOST_AUTO_TEST_SUITE(serialize_tests)
+
+BOOST_AUTO_TEST_CASE(serialize_datastream)
+{
+ CDataStream ds1, ds2, ds3, ds4;
+ ds1 << string("testing") << 123;
+ ds2 << ds1;
+ ds3 += ds1;
+ ds4 = ds1;
+ BOOST_CHECK(ds1.str() == ds2.str());
+ BOOST_CHECK(ds1.str() == ds3.str());
+ BOOST_CHECK(ds1.str() == ds4.str());
+}
+
+BOOST_AUTO_TEST_SUITE_END()
Pretty shocking that there were no pre-existing tests of anything in the whole serialization system, really. (Can something be both shocking and unsurprising at the same time?)
diff -uNr a/bitcoin/src/test/test_bitcoin.cpp b/bitcoin/src/test/test_bitcoin.cpp --- a/bitcoin/src/test/test_bitcoin.cpp 4d8a50d4a2e7f073b219665c6022d68ab1a8aa4d9adc991e253357f5a6bdfb6a091be5eeccd376dc86491442a2f20d4598d721f3732c102d22918abffd27f30f +++ b/bitcoin/src/test/test_bitcoin.cpp 074799dfa9668d680742080c34d3aaf19a71f5c21b436d3e0e5be805664adc436c4d73cf23eb9d05adfe1c9c46fc6dcf5a48a110259306180af5a7805245e137 @@ -14,6 +14,7 @@ #include "base58_tests.cpp" #include "miner_tests.cpp" #include "Checkpoints_tests.cpp" +#include "serialize_tests.cpp" CWallet* pwalletMain;
bitcoin_pushmessage_cleanup_2.vpatch
(top - view patch - browse tree)
Description: With PushMessage deduplicated, we can integrate BeginMessage, AbortMessage and EndMessage for major simplification. While we're here, fix those infernal torn send/receive debug messages with their redundant timestamps and no clue as to sender or recipient.
diff -uNr a/bitcoin/src/main.cpp b/bitcoin/src/main.cpp
--- a/bitcoin/src/main.cpp 5b796065e6bdfce53478af230763776d8342f0da923744cc8f76b9802ad8d01f6bdde0149acc784491130d3033662c9d1602bde39ec213c958cd788cdc3ffef6
+++ b/bitcoin/src/main.cpp 9f194a61889c5bbd661aa84cb485ade9fc6705752e928b9230e9293851a7449d03d9859ca226f3f2c485ec8b506f11436d60123038d20551c63acf1546436284
@@ -1681,8 +1681,7 @@
{
RandAddSeedPerfmon();
if (fDebug) {
- printf("%s ", DateTimeStrFormat("%x %H:%M:%S", GetTime()).c_str());
- printf("received: %s (%d bytes)\n", strCommand.c_str(), vRecv.size());
+ printf("recv %s %s (%d B)\n", pfrom->addr.ToString().c_str(), strCommand.c_str(), vRecv.size());
}
if (mapArgs.count("-dropmessagestest") && GetRand(atoi(mapArgs["-dropmessagestest"])) == 0)
{
Above, as below: what I meant about the debug messages. The date/time is purely redundant because that's added to all "printf" output when fDebug is enabled. Because it printed one line in two separate calls without additional locking, the output of other threads sometimes got mixed in between (log messages were "torn"). By adding the peer address, it becomes possible to trace what's going on with a given peer across multiple messages.
diff -uNr a/bitcoin/src/net.cpp b/bitcoin/src/net.cpp
--- a/bitcoin/src/net.cpp b5ef3c960b24f41ac4cac25eef4dd17f6da3b10d7d404b8048dd30519e994908b11c64f94115cd55a732164c15c4b62ff5f454fe771a5843a7474f239d6f87ee
+++ b/bitcoin/src/net.cpp 2a3a5df4dbdaa976df3229923e229c0c1ffbc2d654b16b3953c5fcc4d8330ae2df5b66a3431d42891ec214710bbdede093c57ac0988b255ba092cafbcae94882
@@ -400,8 +400,6 @@
fDisconnect = true;
if (hSocket != INVALID_SOCKET)
{
- if (fDebug)
- printf("%s ", DateTimeStrFormat("%x %H:%M:%S", GetTime()).c_str());
printf("disconnecting node %s\n", addr.ToString().c_str());
closesocket(hSocket);
hSocket = INVALID_SOCKET;
diff -uNr a/bitcoin/src/net.h b/bitcoin/src/net.h
--- a/bitcoin/src/net.h 446bb5f05b776039fd25680c8b46d8aada9447be55d56d882dbf52a5c1d527b02241a2fdf9985836e344128dbcf6fb3f118afac37b80109867cb05856906a598
+++ b/bitcoin/src/net.h 24f6a340b277bf9b2cc6e13e3d5d65d6324ddd538e121a5fa05dd4c31aeb5bca41c6eaf114a483e1943c7b6f9448afdd174806e81740ad9fa92b8515fd26bbac
@@ -80,8 +80,6 @@
int64 nLastRecv;
int64 nLastSendEmpty;
int64 nTimeConnected;
- unsigned int nHeaderStart;
- unsigned int nMessageStart;
CAddress addr;
int nVersion;
std::string strSubVer;
@@ -127,8 +125,6 @@
nLastRecv = 0;
nLastSendEmpty = GetTime();
nTimeConnected = GetTime();
- nHeaderStart = -1;
- nMessageStart = -1;
addr = addrIn;
nVersion = 0;
strSubVer = "";
@@ -238,81 +234,6 @@
return CDataStream(SER_NETWORK, vSend.GetVersion());
}
- void BeginMessage(const char* pszCommand)
- {
- ENTER_CRITICAL_SECTION(cs_vSend);
- if (nHeaderStart != -1)
- AbortMessage();
- nHeaderStart = vSend.size();
- vSend << CMessageHeader(pszCommand, 0);
- nMessageStart = vSend.size();
- if (fDebug) {
- printf("%s ", DateTimeStrFormat("%x %H:%M:%S", GetTime()).c_str());
- printf("sending: %s ", pszCommand);
- }
- }
-
- void AbortMessage()
- {
- if (nHeaderStart == -1)
- return;
- vSend.resize(nHeaderStart);
- nHeaderStart = -1;
- nMessageStart = -1;
- LEAVE_CRITICAL_SECTION(cs_vSend);
-
- if (fDebug)
- printf("(aborted)\n");
- }
-
- void EndMessage()
- {
- if (mapArgs.count("-dropmessagestest") && GetRand(atoi(mapArgs["-dropmessagestest"])) == 0)
- {
- printf("dropmessages DROPPING SEND MESSAGE\n");
- AbortMessage();
- return;
- }
-
- if (nHeaderStart == -1)
- return;
-
- // Set the size
- unsigned int nSize = vSend.size() - nMessageStart;
- memcpy((char*)&vSend[nHeaderStart] + offsetof(CMessageHeader, nMessageSize), &nSize, sizeof(nSize));
-
- // Set the checksum
- if (vSend.GetVersion() >= 209)
- {
- uint256 hash = Hash(vSend.begin() + nMessageStart, vSend.end());
- unsigned int nChecksum = 0;
- memcpy(&nChecksum, &hash, sizeof(nChecksum));
- assert(nMessageStart - nHeaderStart >= offsetof(CMessageHeader, nChecksum) + sizeof(nChecksum));
- memcpy((char*)&vSend[nHeaderStart] + offsetof(CMessageHeader, nChecksum), &nChecksum, sizeof(nChecksum));
- }
-
- if (fDebug) {
- printf("(%d bytes)\n", nSize);
- }
-
- nHeaderStart = -1;
- nMessageStart = -1;
- LEAVE_CRITICAL_SECTION(cs_vSend);
- }
-
- void EndMessageAbortIfEmpty()
- {
- if (nHeaderStart == -1)
- return;
- int nSize = vSend.size() - nMessageStart;
- if (nSize > 0)
- EndMessage();
- else
- AbortMessage();
- }
-
-
-
void PushVersion()
{
/// when NTP implemented, change to just nTime = GetAdjustedTime()
@@ -331,9 +252,6 @@
<< nBestHeight);
}
-
-
-
void PushMessage(const char* pszCommand)
{
PushMessage(pszCommand, SendingStream());
@@ -341,15 +259,28 @@
void PushMessage(const char* pszCommand, const CDataStream& payload)
{
+ if (fDebug)
+ printf("send %s %s (%d B)\n", addr.ToString().c_str(), pszCommand, payload.size());
+ if (mapArgs.count("-dropmessagestest") && GetRand(atoi(mapArgs["-dropmessagestest"])) == 0)
+ {
+ printf("dropmessages DROPPING SEND MESSAGE %s %s\n", addr.ToString().c_str(), pszCommand);
+ return;
+ }
+ SCOPED_LOCK(cs_vSend);
+ unsigned int nHeaderStart = vSend.size();
try
{
- BeginMessage(pszCommand);
+ CMessageHeader header(pszCommand, payload.size());
+ uint256 hash = Hash(payload.begin(), payload.end());
+ memcpy(&(header.nChecksum), &hash, sizeof header.nChecksum);
+ // If the peer speaks an old protocol version (<209) as indicated by vSend.nVersion, header.nChecksum is excluded from serialization (see CMessageHeader); no need for special handling here as in prior revisions.
+ vSend << header;
vSend += payload;
- EndMessage();
}
catch (...)
{
- AbortMessage();
+ vSend.resize(nHeaderStart);
+ printf("aborted send %s %s\n", addr.ToString().c_str(), pszCommand);
throw;
}
}
It's amazing how much simpler things get once you stop being totally insane, isn't it?(iv)
In particular, the ability to replace that "hash some byte range by pointer maths then patch the result back into another byte range corresponding to a field of some already-serialized object" construct is simply a thing of beauty if I do say so myself, validating the choice to use an intermediate CDataStream for the payload. Moving a couple details outside of the critical section is a nice touch too.
bitcoin_enforce_buffer_limits.vpatch
(top - view patch - browse tree)
Description: Finally we can move send and receive flood control logic to where the buffers get filled, so that the rest of the code can count on their not exceeding the size limits. Passing the error upstack is needed in the sending case to stop whatever was generating the output. While we're here, fix an erroneous fcntl flag bit inversion and kill a 64k stack buffer.
diff -uNr a/bitcoin/src/main.cpp b/bitcoin/src/main.cpp
--- a/bitcoin/src/main.cpp 9f194a61889c5bbd661aa84cb485ade9fc6705752e928b9230e9293851a7449d03d9859ca226f3f2c485ec8b506f11436d60123038d20551c63acf1546436284
+++ b/bitcoin/src/main.cpp ae05983cfd5fabb8bc16e6e6ce5dfd302be9a4546302837de972ea92600f296312fe4535c84da0a991bf34fbe02ee24399201b439ee7eb33f20ed1581d7c9e34
@@ -2228,14 +2228,9 @@
}
else
{
- PrintExceptionContinue(&e, "ProcessMessage()");
+ throw;
}
}
- catch (std::exception& e) {
- PrintExceptionContinue(&e, "ProcessMessage()");
- } catch (...) {
- PrintExceptionContinue(NULL, "ProcessMessage()");
- }
if (!fRet)
printf("ProcessMessage(%s, %u bytes) FAILED\n", strCommand.c_str(), nMessageSize);
The above is in ProcessMessages which handles all fully received messages in the buffer for a given peer. There's no strong need to continue serving that peer right now if an uncaught exception comes up this far. If it doesn't get disconnected, then we'll get back to serving it in its turn soon enough; on the other hand if the exception itself indicates disconnection, we don't want to keep trying to send it stuff. (Orderly disconnection doesn't use exceptions.)
diff -uNr a/bitcoin/src/net.cpp b/bitcoin/src/net.cpp
--- a/bitcoin/src/net.cpp 2a3a5df4dbdaa976df3229923e229c0c1ffbc2d654b16b3953c5fcc4d8330ae2df5b66a3431d42891ec214710bbdede093c57ac0988b255ba092cafbcae94882
+++ b/bitcoin/src/net.cpp 73d3ab5906c00b2e7fc030b8664404d91ddea987ca9899dc30abda12e9e9e17966ad1abfbf5587753053231f8125e75a38f5f8d9d68c023dc92110ebfb1cd398
@@ -143,7 +143,7 @@
but we'll turn it back to blocking just in case
*/
fFlags = fcntl(hSocket, F_GETFL, 0);
- if (fcntl(hSocket, F_SETFL, fFlags & !O_NONBLOCK) == SOCKET_ERROR)
+ if (fcntl(hSocket, F_SETFL, fFlags & ~O_NONBLOCK) == SOCKET_ERROR)
Bitwise rather than logical inversion was clearly the intent here. Looking at the implications of the mistake: O_NONBLOCK is nonzero on my system (and presumably all systems, since it's a flag bit), so !O_NONBLOCK is zero, so all the file status flags (F_SETFL) are cleared rather than just O_NONBLOCK. What might these flags be? My local Linux fcntl(2) man page seems to indicate O_APPEND, O_ASYNC, O_DIRECT, O_NOATIME and O_NONBLOCK as the complete list of those that can be changed. Of these, O_APPEND, O_DIRECT and O_NOATIME don't pertain to sockets as far as I can tell; O_ASYNC is for signal-driven I/O and is off by default (incidentally, I found it sucks horribly on Linux, a sad story for perhaps another day). Thus, the bug appears benign in this context, and in general the flags seem to follow an off-by-default pattern; but it's unclear what the limits of these assumptions may be so for the long run we're certainly better off fixing it.
{
closesocket(hSocket);
return false;
@@ -667,17 +667,21 @@
CDataStream& vRecv = pnode->vRecv;
unsigned int nPos = vRecv.size();
- if (nPos > ReceiveBufferSize()) {
- if (!pnode->fDisconnect)
- printf("socket recv flood control disconnect (%d bytes)\n", vRecv.size());
- pnode->CloseSocketDisconnect();
- }
- else {
+ {
+ static CCriticalSection cs_recv_buffer;
+ SCOPED_LOCK(cs_recv_buffer);
+ // Static storage is OK because there's only one ThreadSocketHandler; the lock is just for safety in case this were to change. Large stack buffers on the other hand can be risky, especially with multithreading since address space for guard pages is more limited.
// typical socket buffer is 8K-64K
- char pchBuf[0x10000];
+ static char pchBuf[0x10000];
int nBytes = recv(pnode->hSocket, pchBuf, sizeof(pchBuf), MSG_DONTWAIT);
if (nBytes > 0)
{
Oh yes, the receiving buffer's configured limit could be overflowed too, though the effect wasn't particularly harmful as the amount was limited to the 64K of the above pchBuf.
+ if ((unsigned) nBytes > ReceiveBufferSize() - nPos) { // this form rules out addition overflow
+ if (!pnode->fDisconnect)
+ printf("socket recv flood control disconnect (%d B)\n", nPos + nBytes);
+ pnode->CloseSocketDisconnect();
+ continue;
+ }
vRecv.resize(nPos + nBytes);
memcpy(&vRecv[nPos], pchBuf, nBytes);
pnode->nLastRecv = GetTime();
@@ -732,11 +736,6 @@
pnode->CloseSocketDisconnect();
}
}
- if (vSend.size() > SendBufferSize()) {
- if (!pnode->fDisconnect)
- printf("socket send flood control disconnect (%d bytes)\n", vSend.size());
- pnode->CloseSocketDisconnect();
- }
}
}
}
Below, the exception handling lifted upstack from ProcessMessages, now with the added case of peer_disconnected_error. Observe that we "continue" the vNodesCopy iteration meaning that we do NOT continue to SendMessages for the departed peer. The move of the fShutdown check to first thing in the loop is so it's not bypassed on "continue".
@@ -1004,16 +1003,40 @@
BOOST_FOREACH(CNode* pnode, vNodesCopy)
{
// Receive messages
- TRY_CRITICAL_BLOCK(pnode->cs_vRecv)
- ProcessMessages(pnode);
if (fShutdown)
return;
+ try
+ {
+ TRY_CRITICAL_BLOCK(pnode->cs_vRecv)
+ ProcessMessages(pnode);
+ }
+ catch (peer_disconnected_error& e)
+ {
+ printf("ProcessMessages() : peer disconnected\n");
+ continue;
+ }
+ catch (std::exception& e)
+ {
+ PrintExceptionContinue(&e, "ProcessMessages()");
+ }
+ catch (...)
+ {
+ PrintExceptionContinue(NULL, "ProcessMessages()");
+ }
// Send messages
- TRY_CRITICAL_BLOCK(pnode->cs_vSend)
- SendMessages(pnode, pnode == pnodeTrickle);
if (fShutdown)
return;
+ try
+ {
+ TRY_CRITICAL_BLOCK(pnode->cs_vSend)
+ SendMessages(pnode, pnode == pnodeTrickle);
+ }
+ catch (peer_disconnected_error& e)
+ {
+ printf("SendMessages() : peer disconnected\n");
+ continue;
+ }
}
CRITICAL_BLOCK(cs_vNodes)
Below, the new exception class as seen in action above. Distinguishing it as a separate machine-recognizable type of error follows in part from the Unix approach where writing to a closed pipe or socket generates an EPIPE error (or even a SIGPIPE signal, for the unwary), besides just good programming practice, tempting as it was to just throw std::runtime_error("disconnected!11") and not have to look up how to do the inheritance thing.
diff -uNr a/bitcoin/src/net.h b/bitcoin/src/net.h
--- a/bitcoin/src/net.h 24f6a340b277bf9b2cc6e13e3d5d65d6324ddd538e121a5fa05dd4c31aeb5bca41c6eaf114a483e1943c7b6f9448afdd174806e81740ad9fa92b8515fd26bbac
+++ b/bitcoin/src/net.h 245392b337f179b07eb142d705f6d90e7cc376d0158c80c156eb6b7aee8adcf90e58eca4982266a7ebcae458a27e02da0e8ef54b279dcf2b1856b29b15e2222e
@@ -61,6 +61,11 @@
extern int fUseProxy;
extern CAddress addrProxy;
+class peer_disconnected_error : public std::runtime_error
+{
+public:
+ explicit peer_disconnected_error() : std::runtime_error("write to disconnected peer") {}
+};
@@ -142,7 +147,16 @@
// Be shy and don't send version until we hear
if (!fInbound)
- PushVersion();
+ {
+ try
+ {
+ PushVersion();
+ }
+ catch (peer_disconnected_error& e)
+ {
+ // safe to ignore for this single message, and unclear that callers are prepared to handle
+ }
+ }
}
~CNode()
Finally the keystone: we modify the now deduplicated and simplified PushMessage to enforce the sending buffer limit.
@@ -266,7 +280,12 @@
printf("dropmessages DROPPING SEND MESSAGE %s %s\n", addr.ToString().c_str(), pszCommand);
return;
}
+
SCOPED_LOCK(cs_vSend);
+ if (fDisconnect)
+ // Keeping this check inside the lock seems preferable as it assures that past this point, the peer was not disconnected due to flood control (though it may still be for other reasons as fDisconnect isn't strictly guarded by cs_vSend).
+ throw peer_disconnected_error();
+
unsigned int nHeaderStart = vSend.size();
try
{
@@ -274,6 +293,7 @@
uint256 hash = Hash(payload.begin(), payload.end());
memcpy(&(header.nChecksum), &hash, sizeof header.nChecksum);
// If the peer speaks an old protocol version (<209) as indicated by vSend.nVersion, header.nChecksum is excluded from serialization (see CMessageHeader); no need for special handling here as in prior revisions.
+ // The following may temporarily overflow the sending buffer size limit; that's OK because we rectify it before releasing cs_vSend.
vSend << header;
vSend += payload;
}
@@ -283,6 +303,14 @@
printf("aborted send %s %s\n", addr.ToString().c_str(), pszCommand);
throw;
}
+
+ if (vSend.size() > SendBufferSize())
+ {
+ printf("socket send flood control disconnect (%d B)\n", vSend.size());
+ vSend.clear();
+ CloseSocketDisconnect();
+ throw peer_disconnected_error();
+ }
}
void PushGetBlocks(CBlockIndex* pindexBegin, uint256 hashEnd);
It's not presently clear to me that the pre-check of fDisconnect is strictly necessary ; it probably flows from the previous code and an abundance of caution. At first I hadn't even bothered with the exception throwing, figuring that returning after clearing the buffer and disconnecting would be sufficient. Testing with the attack script revealed that while the hard lockup was indeed cured and the attacker disconnected, the victim node could still be weighed down for a while, fetching, checksumming, logging and attempting to send the flood of previously requested blocks.
bitcoin_posix_error_handling.vpatch
(top - view patch - browse tree)
While not as critical as the preceeding, this is a patch that I personally wouldn't want to do without, now that I've done it - and didn't want to not do, once I'd done the looking. Especially if non-Linux-on-Intel systems are in the mix.
Description:
- Convert all error codes to readable strings in log and console output.
- Don't log numeric fork/setsid result in the error case as it's always -1 and only errno is interesting.
- Translate socket error handling from WinSock to better fit unix-like systems: in particular, EAGAIN may be distinct from EWOULDBLOCK; EWOULDBLOCK and EINVAL don't apply to connect; EINPROGRESS doesn't apply to send/recv; EMSGSIZE doesn't seem to apply to TCP and it's unclear that it could be handled if it did; and errno needs to be saved when there may be intervening libc calls (nAcceptErr).
diff -uNr a/bitcoin/src/init.cpp b/bitcoin/src/init.cpp
--- a/bitcoin/src/init.cpp 65d10b2fb7d554c0833d97931fde3429dad8a61d1c8e5a9c3d590d67340c43356a35a3ebcce1f4a96eee9e13b1b359a2173814cc97bccdfe6fd6a7d94ea0f74a
+++ b/bitcoin/src/init.cpp 830f8ff84148b7cefa7b846e9e4fba03f60a1a68dc7066f1bb00b56dc4c4899056364dfd016869489d3567dfc6b2cc043e7581fb17b514dfd3ee08e10bb80db4
@@ -272,20 +272,21 @@
{
// Daemonize
pid_t pid = fork();
- if (pid < 0)
+ if (pid == -1)
{
- fprintf(stderr, "Error: fork() returned %d errno %d\n", pid, errno);
+ fprintf(stderr, "Error: fork(): %s\n", StringError(errno).c_str());
return false;
}
- if (pid > 0)
+ if (pid)
{
+ // parent
CreatePidFile(GetPidFile(), pid);
return true;
}
+ // child
- pid_t sid = setsid();
- if (sid < 0)
- fprintf(stderr, "Error: setsid() returned %d errno %d\n", sid, errno);
+ if (setsid() == -1)
+ fprintf(stderr, "Error: setsid(): %s\n", StringError(errno).c_str());
}
if (!fDebug && !pszSetDataDir[0])
diff -uNr a/bitcoin/src/net.cpp b/bitcoin/src/net.cpp
--- a/bitcoin/src/net.cpp 73d3ab5906c00b2e7fc030b8664404d91ddea987ca9899dc30abda12e9e9e17966ad1abfbf5587753053231f8125e75a38f5f8d9d68c023dc92110ebfb1cd398
+++ b/bitcoin/src/net.cpp 15bb33eac15ff9e8d26a6a02ac68936bf748be6edd6f55a860edf8dc2b470ac9bc5e3cccd076164e82b6c9c282956820b2b3466b6c77dc77651d835e07732853
@@ -92,8 +92,7 @@
if (connect(hSocket, (struct sockaddr*)&sockaddr, sizeof(sockaddr)) == SOCKET_ERROR)
{
- // WSAEINVAL is here because some legacy version of winsock uses it
- if (WSAGetLastError() == WSAEINPROGRESS || WSAGetLastError() == WSAEWOULDBLOCK || WSAGetLastError() == WSAEINVAL)
+ if (errno == EINPROGRESS)
{
struct timeval timeout;
timeout.tv_sec = nTimeout / 1000;
@@ -111,27 +110,27 @@
}
if (nRet == SOCKET_ERROR)
{
- printf("select() for connection failed: %i\n",WSAGetLastError());
+ printf("select() for connection failed: %s\n", StringError(errno).c_str());
closesocket(hSocket);
return false;
}
socklen_t nRetSize = sizeof(nRet);
if (getsockopt(hSocket, SOL_SOCKET, SO_ERROR, &nRet, &nRetSize) == SOCKET_ERROR)
{
- printf("getsockopt() for connection failed: %i\n",WSAGetLastError());
+ printf("getsockopt() for connection failed: %s\n", StringError(errno).c_str());
closesocket(hSocket);
return false;
}
if (nRet != 0)
{
- printf("connect() failed after select(): %s\n",strerror(nRet));
+ printf("connect() failed after select(): %s\n", StringError(nRet).c_str());
closesocket(hSocket);
return false;
}
}
else
{
- printf("connect() failed: %i\n",WSAGetLastError());
+ printf("connect() failed: %s\n", StringError(errno).c_str());
closesocket(hSocket);
return false;
}
@@ -375,7 +374,7 @@
// Set to nonblocking
if (fcntl(hSocket, F_SETFL, O_NONBLOCK) == SOCKET_ERROR)
- printf("ConnectSocket() : fcntl nonblocking setting failed, error %d\n", errno);
+ printf("ConnectSocket() : fcntl nonblocking setting failed: %s\n", StringError(errno).c_str());
// Add node
CNode* pnode = new CNode(hSocket, addrConnect, false);
@@ -583,10 +582,9 @@
return;
if (nSelect == SOCKET_ERROR)
{
- int nErr = WSAGetLastError();
if (hSocketMax > -1)
{
- printf("socket select error %d\n", nErr);
+ printf("socket select error: %s\n", StringError(errno).c_str());
for (int i = 0; i <= hSocketMax; i++)
FD_SET(i, &fdsetRecv);
}
@@ -604,6 +602,7 @@
struct sockaddr_in sockaddr;
socklen_t len = sizeof(sockaddr);
SOCKET hSocket = accept(hListenSocket, (struct sockaddr*)&sockaddr, &len);
+ int nAcceptErr = errno;
CAddress addr;
int nInbound = 0;
@@ -617,8 +616,8 @@
if (hSocket == INVALID_SOCKET)
{
- if (WSAGetLastError() != WSAEWOULDBLOCK)
- printf("socket error accept failed: %d\n", WSAGetLastError());
+ if (nAcceptErr != EWOULDBLOCK && nAcceptErr != EAGAIN)
+ printf("socket error accept failed: %s\n", StringError(nAcceptErr).c_str());
}
else if (nInbound >= GetArg("-maxconnections", 125) - MAX_OUTBOUND_CONNECTIONS)
{
@@ -696,11 +695,10 @@
else if (nBytes < 0)
{
// error
- int nErr = WSAGetLastError();
- if (nErr != WSAEWOULDBLOCK && nErr != WSAEMSGSIZE && nErr != WSAEINTR && nErr != WSAEINPROGRESS)
+ if (errno != EWOULDBLOCK && errno != EAGAIN && errno != EINTR)
{
if (!pnode->fDisconnect)
- printf("socket recv error %d\n", nErr);
+ printf("socket recv error: %s\n", StringError(errno).c_str());
pnode->CloseSocketDisconnect();
}
}
@@ -729,10 +727,9 @@
else if (nBytes < 0)
{
// error
- int nErr = WSAGetLastError();
- if (nErr != WSAEWOULDBLOCK && nErr != WSAEMSGSIZE && nErr != WSAEINTR && nErr != WSAEINPROGRESS)
+ if (errno != EWOULDBLOCK && errno != EAGAIN && errno != EINTR)
{
- printf("socket send error %d\n", nErr);
+ printf("socket send error: %s\n", StringError(errno).c_str());
pnode->CloseSocketDisconnect();
}
}
@@ -1073,7 +1070,7 @@
hListenSocket = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
if (hListenSocket == INVALID_SOCKET)
{
- strError = strprintf("Error: Couldn't open socket for incoming connections (socket returned error %d)", WSAGetLastError());
+ strError = string("Error: Couldn't open socket for incoming connections: ") + StringError(errno);
printf("%s\n", strError.c_str());
return false;
}
@@ -1089,7 +1086,7 @@
if (fcntl(hListenSocket, F_SETFL, O_NONBLOCK) == SOCKET_ERROR)
{
- strError = strprintf("Error: Couldn't set properties on socket for incoming connections (error %d)", WSAGetLastError());
+ strError = string("Error: Couldn't set properties on socket for incoming connections: ") + StringError(errno);
printf("%s\n", strError.c_str());
return false;
}
@@ -1103,11 +1100,7 @@
sockaddr.sin_port = htons(GetListenPort());
if (::bind(hListenSocket, (struct sockaddr*)&sockaddr, sizeof(sockaddr)) == SOCKET_ERROR)
{
- int nErr = WSAGetLastError();
- if (nErr == WSAEADDRINUSE)
- strError = strprintf(_("Unable to bind to port %d on this computer. Bitcoin is probably already running."), ntohs(sockaddr.sin_port));
- else
- strError = strprintf("Error: Unable to bind to port %d on this computer (bind returned error %d)", ntohs(sockaddr.sin_port), nErr);
+ strError = strprintf("Error: Unable to bind to port %d on this computer: ", ntohs(sockaddr.sin_port)) + StringError(errno);
printf("%s\n", strError.c_str());
return false;
}
@@ -1116,7 +1109,7 @@
// Listen for incoming connections
if (listen(hListenSocket, SOMAXCONN) == SOCKET_ERROR)
{
- strError = strprintf("Error: Listening for incoming connections failed (listen returned error %d)", WSAGetLastError());
+ strError = string("Error: Listening for incoming connections failed: ") + StringError(errno);
printf("%s\n", strError.c_str());
return false;
}
@@ -1241,7 +1234,7 @@
closesocket(pnode->hSocket);
if (hListenSocket != INVALID_SOCKET)
if (closesocket(hListenSocket) == SOCKET_ERROR)
- printf("closesocket(hListenSocket) failed with error %d\n", WSAGetLastError());
+ printf("closesocket(hListenSocket) failed: %s\n", StringError(errno).c_str());
}
}
diff -uNr a/bitcoin/src/util.cpp b/bitcoin/src/util.cpp
--- a/bitcoin/src/util.cpp d0d39dab1148e544ba93766ae80efcf004b715a0af89db3e98f0ea14b842c683f4f1bc6df6182347b36482aacc52c120d7e4beaf6128df44cbbfa4d9b583959d
+++ b/bitcoin/src/util.cpp 15b46344c80e58143a11b215de7c883167142ff65136bb90df78ccdb01ca3a2a1fb1ebc61f78fea88c17c504b209f87eba7b42b2ae8d3a67652bfd133a97a850
@@ -907,6 +907,18 @@
return ss.str();
}
+std::string StringError(int nErr)
+{
+ {
+ static CCriticalSection cs_strerror;
+ SCOPED_LOCK(cs_strerror);
+ // Locking makes this thread-safe, but not fully reentrant (async signal safe). strerror_r is a pain due to a POSIX vs GNU conflict.
+ char *s = strerror(nErr);
+ if (s)
+ return std::string(s);
+ }
+ return strprintf("Unknown error %d", nErr);
+}
#ifdef DEBUG_LOCKORDER
diff -uNr a/bitcoin/src/util.h b/bitcoin/src/util.h
--- a/bitcoin/src/util.h 25e0005b1ac501d4058966ec78823ff48427dc8dcb39f6414cba439782da31c5de505bf931ea41fee35bd3327caf0a90af74bb4a5a83617ab0d93caea61c5357
+++ b/bitcoin/src/util.h bab7740fd689486507200267b17cb0db5e4e082ccdbd6057915f723b778a8880443af98cd610e1d0094d944e6634c31431cb701ed87e16cb694938c7bd4f5020
@@ -65,18 +65,9 @@
return u.ptr;
}
-#define WSAGetLastError() errno
-#define WSAEINVAL EINVAL
-#define WSAEALREADY EALREADY
-#define WSAEWOULDBLOCK EWOULDBLOCK
-#define WSAEMSGSIZE EMSGSIZE
-#define WSAEINTR EINTR
-#define WSAEINPROGRESS EINPROGRESS
-#define WSAEADDRINUSE EADDRINUSE
-#define WSAENOTSOCK EBADF
-#define INVALID_SOCKET (SOCKET)(~0)
+#define INVALID_SOCKET -1
#define SOCKET_ERROR -1
-typedef u_int SOCKET;
+typedef int SOCKET;
#define _vsnprintf(a,b,c,d) vsnprintf(a,b,c,d)
#define strlwr(psz) to_lower(psz)
#define _strlwr(psz) to_lower(psz)
@@ -90,15 +81,12 @@
}
-inline int myclosesocket(SOCKET& hSocket)
+inline int closesocket(SOCKET& hSocket)
{
- if (hSocket == INVALID_SOCKET)
- return WSAENOTSOCK;
int ret = close(hSocket);
hSocket = INVALID_SOCKET;
return ret;
}
-#define closesocket(s) myclosesocket(s)
inline const char* _(const char* psz)
{
return psz;
@@ -167,6 +155,7 @@
void AddTimeData(unsigned int ip, int64 nTime);
std::string FormatFullVersion();
std::string FormatSubVersion(const std::string& name, int nClientVersion);
+std::string StringError(int nErr);
bitcoin_rebranding.vpatch
(top - view patch - browse tree)
It's about time we updated that homing signal. Let's see who's following along!
Description: Change version string to jwrd.net; apparently even something this simple can't be done without bumping into shoddy earlier work.
diff -uNr a/bitcoin/src/init.cpp b/bitcoin/src/init.cpp
--- a/bitcoin/src/init.cpp 830f8ff84148b7cefa7b846e9e4fba03f60a1a68dc7066f1bb00b56dc4c4899056364dfd016869489d3567dfc6b2cc043e7581fb17b514dfd3ee08e10bb80db4
+++ b/bitcoin/src/init.cpp 98181df7de317c09aa491d95f0320cbf6afbe8fae3bf8b1bf88457a2309ff67c7e45ac9538e89e2de53a7759e311feb4b13fdba7514acf6a54768b29467d5f34
@@ -217,12 +217,12 @@
if (mapArgs.count("-setverstring"))
{
- CLIENT_NAME = mapArgs["-setverstring"];
+ strClientVersionName = mapArgs["-setverstring"];
}
if (mapArgs.count("-setvernum"))
{
- VERSION = atoi(mapArgs["-setvernum"]);
+ nClientVersionNum = atoi(mapArgs["-setvernum"]);
}
if (fDaemon)
diff -uNr a/bitcoin/src/knobs.h b/bitcoin/src/knobs.h
--- a/bitcoin/src/knobs.h 4e01d59f75dfdac897735bf28c2414ed8463e12683e1d845272772feac7706615e36c26e6e291250d245f269c2bf8b41a32cc6abb81bbc58b4831b01be0eefd8
+++ b/bitcoin/src/knobs.h false
@@ -1,7 +0,0 @@
-#ifndef KNOBS_H
-#define KNOBS_H
-
-#define DEFAULT_CLIENT_NAME "therealbitcoin.org"
-#define DEFAULT_CLIENT_VERSION 99999 /* 50400 */
-
-#endif
diff -uNr a/bitcoin/src/main.cpp b/bitcoin/src/main.cpp
--- a/bitcoin/src/main.cpp ae05983cfd5fabb8bc16e6e6ce5dfd302be9a4546302837de972ea92600f296312fe4535c84da0a991bf34fbe02ee24399201b439ee7eb33f20ed1581d7c9e34
+++ b/bitcoin/src/main.cpp 45cf30812854a51d7a4c3436b8a832c8418dbb03a70b6bc6e81fba21c90920355e93d2d62996b6d4d643b19dea997a6df11d50884567d3afc98be8782fa66685
@@ -19,7 +19,9 @@
// Global state
//
-int VERSION = DEFAULT_CLIENT_VERSION;
+// This verbose variable naming is to rule out confusion with prior local names such as "nClientVersion".
+int nClientVersionNum = 99999; // Was 50400 prior to programmable-versionstring patch. May affect network protocol, serialization, peer acceptance and such.
+std::string strClientVersionName("jwrd.net");
CCriticalSection cs_setpwalletRegistered;
set<CWallet*> setpwalletRegistered;
diff -uNr a/bitcoin/src/main.h b/bitcoin/src/main.h
--- a/bitcoin/src/main.h d70c7dd93b2bea85ffe52d440b60d629c4cf125d9ae91afef3aa6c854981c19070663d3a8839455b700e410435634f59fb3ccaf7642595e4374c4bb323261267
+++ b/bitcoin/src/main.h aca54ef12a38a60aaa5aa4cf466cc6514d35429d041db06352175e4ed73b265bae7d1b87131225b860f7dda5e6815a02b525d555407d82c5d4c5cd2974e0f2d1
@@ -65,6 +65,10 @@
extern int fMinimizeToTray;
extern int fMinimizeOnClose;
extern int nCheckBlocks;
+extern int nClientVersionNum;
+// This didn't used to be settable; there's still many references to the all-caps form, and moreover its meaning seems rather overloaded and it may be that some really should be constant (such as internal usage in db.cpp) and others settable.
+#define VERSION nClientVersionNum
+extern std::string strClientVersionName;
int64 GetTransactionFee();
void SetTransactionFee(int64 nFee);
diff -uNr a/bitcoin/src/net.h b/bitcoin/src/net.h
--- a/bitcoin/src/net.h 245392b337f179b07eb142d705f6d90e7cc376d0158c80c156eb6b7aee8adcf90e58eca4982266a7ebcae458a27e02da0e8ef54b279dcf2b1856b29b15e2222e
+++ b/bitcoin/src/net.h 11a23408b4a7562a897755a96485957c61d11633a9ee5783fa950c0f6653ee69f1e7cc338963c1e694a994fa8f9f9dd07ad414512f3d607389a5770567141235
@@ -61,6 +61,10 @@
extern int fUseProxy;
extern CAddress addrProxy;
+// Duplicated from main.h, stupid code-in-header-files.
+extern int nClientVersionNum;
+extern std::string strClientVersionName;
+
class peer_disconnected_error : public std::runtime_error
{
public:
@@ -256,13 +260,13 @@
CAddress addrMe = (fUseProxy || !addrLocalHost.IsRoutable() ? CAddress("0.0.0.0") : addrLocalHost);
RAND_bytes((unsigned char*)&nLocalHostNonce, sizeof(nLocalHostNonce));
PushMessage("version", SendingStream()
- << VERSION
+ << nClientVersionNum
<< nLocalServices
<< nTime
<< addrYou
<< addrMe
<< nLocalHostNonce
- << FormatSubVersion(CLIENT_NAME, VERSION)
+ << FormatSubVersion(strClientVersionName, nClientVersionNum)
<< nBestHeight);
}
diff -uNr a/bitcoin/src/serialize.h b/bitcoin/src/serialize.h
--- a/bitcoin/src/serialize.h 72b2dc097c54ad0e89545676d555c92da97ee6d86b6a7fd9c2b063ad629b027704f791e0b4168a0d33766f8203b7297f95d274627c89be974c749143949c86ef
+++ b/bitcoin/src/serialize.h 2f6f83b35121f6bd33b405265497c5e5737e7bce3f842992e580ffbcd98f0acf3e07e5a0ffd7612996b73e8a05f9f3314cce1e3596921210711870d18472a9f6
@@ -25,8 +25,6 @@
#include <sys/mman.h>
#include <limits.h>
-#include "knobs.h"
-
/* This comes from limits.h if it's not defined there set a sane default */
#ifndef PAGESIZE
#include <unistd.h>
@@ -44,7 +42,9 @@
class CAutoFile;
static const unsigned int MAX_SIZE = 0x02000000;
-extern int VERSION;
+// Duplicated from main.h, what can you do.
+extern int nClientVersionNum;
+#define VERSION nClientVersionNum
// Used to bypass the rule against non-const reference to temporary
// where it makes sense with wrappers such as CFlatData or CTxDB
diff -uNr a/bitcoin/src/util.cpp b/bitcoin/src/util.cpp
--- a/bitcoin/src/util.cpp 15b46344c80e58143a11b215de7c883167142ff65136bb90df78ccdb01ca3a2a1fb1ebc61f78fea88c17c504b209f87eba7b42b2ae8d3a67652bfd133a97a850
+++ b/bitcoin/src/util.cpp 7c70391a712c509983ac2a57e259ea8e834e54d81de1abc599dd9ef315373ce5c1dbcb242bcafd817ff2527f5a4ba98891145dcc1c47fbb4d6556061b2eb50d1
@@ -2,7 +2,6 @@
// Copyright (c) 2009-2012 The Bitcoin developers
// Distributed under the MIT/X11 software license, see the accompanying
// file license.txt or http://www.opensource.org/licenses/mit-license.php.
-#include "knobs.h"
#include "headers.h"
#include "strlcpy.h"
#include <boost/program_options/detail/config_file.hpp>
@@ -37,8 +36,6 @@
bool fDisableSafeMode = false;
bool fPermissive = false;
-std::string CLIENT_NAME(DEFAULT_CLIENT_NAME);
-
// Workaround for "multiple definition of `_tls_used'"
// http://svn.boost.org/trac/boost/ticket/4258
@@ -884,7 +881,7 @@
-string FormatVersion(int nVersion)
+static std::string FormatVersion(int nVersion)
{
if (nVersion%100 == 0)
return strprintf("%d.%d.%d", nVersion/1000000, (nVersion/10000)%100, (nVersion/100)%100);
@@ -892,17 +889,18 @@
return strprintf("%d.%d.%d.%d", nVersion/1000000, (nVersion/10000)%100, (nVersion/100)%100, nVersion%100);
}
-string FormatFullVersion()
+std::string FormatFullVersion()
{
- string s = FormatVersion(VERSION);
- return s;
+ // Displayed in JSON-RPC headers, help, and log output. Formerly appended local version suffixes like "-beta".
+ return FormatVersion(nClientVersionNum);
}
-std::string FormatSubVersion(const std::string& name, int nClientVersion)
+std::string FormatSubVersion(const std::string& strName, int nVersion)
{
+ // Sent in the "version" handshake. The "Sub" naming seems to reflect the origins of that field as a default-empty "pszSubVer" before being recast as a user-agent string with this slashified format.
std::ostringstream ss;
ss << "/";
- ss << name << ":" << FormatVersion(nClientVersion);
+ ss << strName << ":" << FormatVersion(nVersion);
ss << "/";
return ss.str();
}
diff -uNr a/bitcoin/src/util.h b/bitcoin/src/util.h
--- a/bitcoin/src/util.h bab7740fd689486507200267b17cb0db5e4e082ccdbd6057915f723b778a8880443af98cd610e1d0094d944e6634c31431cb701ed87e16cb694938c7bd4f5020
+++ b/bitcoin/src/util.h 69eabb88785ef1d9be04a591fc11fd8055f2e805ca93a8f9c37768101b3b548a3034c7606207215e6d508747f6a6528b09632e2dd22c2f399e14d02c4ee8885f
@@ -108,7 +108,6 @@
extern std::string strMiscWarning;
extern bool fNoListen;
extern bool fLogTimestamps;
-extern std::string CLIENT_NAME;
extern bool fLowS;
extern bool fHighS;
extern bool fTestSafeMode;
@@ -154,7 +153,7 @@
int64 GetAdjustedTime();
void AddTimeData(unsigned int ip, int64 nTime);
std::string FormatFullVersion();
-std::string FormatSubVersion(const std::string& name, int nClientVersion);
+std::string FormatSubVersion(const std::string& strName, int nVersion);
std::string StringError(int nErr);
In conclusion... oh, what conclusion? Just go try it out and stop living in quite so much squalor already! Or if you're at a loss for how to make use of it all, head on over to jwrd.net and let's see what we can do for you.
- Prior attempted solutions were so superficial as to scarcely be worth another mention, other than to note the following for the benefit of any confused onlookers :
No, mod6_getdata_blocks_wrangler.vpatch - described by its author as "strictly experimental" and to this day still not included in the clown-foundation's official patch set or instructions - does no more than paper over the problem, blocking only the particular attack variant that was most easily reproduced.
And yes, I'm able to freeze up such "patched" nodes, remotely, on demand and with only moderate bandwidth requirements. The updated attack is still a bit rough and may not be fully reliable but there's room for improvement there too! [^]
- Some exceptions were a series presenting a new program while it was still somewhat in flux, my cautious first step into proposing changes to the big scary TRB, and an in-depth review of a desired patch from an unknown entity. [^]
- The joke is that in my formative years, "C++0x" was the working name of the promised next-generation standard, slated to come out some time within the '00s. It didn't get out until 2011. Now there's a C++14; at least some subset may be usable in my world, but the full thing didn't make it into the window of acceptable GCC versions. [^]
- Now why do I recall using almost those exact words somewhere else recently? Perhaps it was from thinking ahead to this article, or some other insanity altogether. [^]
Win.
And by my count, 18 of those 251 insertions were comments. As I mentioned in the log, this was a fun read with the various layers of commentary you weaved together.
I like the new log output.
It's nice to have the article point that out at the specific spot it's happening.
Alright, makes sense to me.
Lol, bloody in the right spots !
Yeah, good to clean out that cruft.
Yeah, good to note the dead ends too.
Perhaps if you've had a history of being shocked by a particular source, the most recent just reminds you of and reinforces the history.
Yeah, the new log is much nicer now. We get a clearer look at the conversation with our peers. Gigabytes of previously inscrutable logs now have some meaning.
Word.
Interesting.
Definitely in the top five of double negatives I've come across, lolz. Cheers to the looking and the doing !
Socking the winsock is a win, I say !
I've got it pressed, built and running. Cheers.
Comment by Robinson Dorion — 2022-05-25 @ 18:27
Thanks.
Odd that "doing without" doesn't seem to have a natural non-negative form, so I had to make do without one.
Comment by Jacob Welsh — 2022-05-25 @ 22:46
jfw, why add "SCOPED_LOCK()"? Seems to me like an unnecessary new item to study, when already familiar, "ENTER_CRITICAL_SECTION()" and "LEAVE_CRITICAL_SECTION()" items would've (afaict) worked the same.
Comment by cgra — 2022-06-07 @ 13:24
See here ; it was added in the transaction fee cleanup patch when I had cause to look at the locking constructs.
The closer comparison would be to CRITICAL_BLOCK rather than ENTER/LEAVE_CRITICAL_SECTION. The main reasons to prefer the first to the second I'd say are that it leverages builtin language syntax and semantics, making for less code to keep track of (would you really prefer to malloc+free when stack allocation would suffice?), and that it does the right thing on nonlocal exits (eg return, goto, or exception being thrown which can happen in unexpected places).
Then compare CRITICAL_BLOCK with SCOPED_LOCK implementations and look at it from the other side: why on earth does the first involve an if-statement, wtf does a bare object evaluate to in boolean context, what's the scope of such an object declared inside the condition of such a statement, and how is it justified that the reader even has to ask these questions ?
Comment by Jacob Welsh — 2022-06-08 @ 02:21
jfw, thank you for your thorough response!
So I had already forgotten these macros were Satoshi's handwriting, and not Boost items, and was thinking "why bring in more Boost?" when asking my question.
Re: CRITICAL_BLOCK vs ENTER/LEAVE_CRITICAL_SECTION: I completely agree, I was apparently somehow still stuck in my head with the old composition of the code where the ENTER/LEAVE were necessary.
Re: CRITICAL_BLOCK implementation: history-aware reader is doomed to answer those (annoying) questions anyway, so in that sense, I don't see much gain.
Fwiw, I would've perhaps used the name "CRITICAL_SCOPE", for consistency.
Comment by cgra — 2022-06-08 @ 08:41
I'm not sure what you refer to there - something pre-0.5.3 ?
I'm not sure about that either ; presumably it would depend on what he wants to know. If the goal is to verify that a new version has fully equivalent behavior to an old one then I can make it easy: it does not, because the old one was broken in however-many ways. What's more important not to change is the block validation rules (which is why I'm more cautious about ripping out e.g. BOOST_FOREACH) ; but even there to some extent we have to compare with the "ideal form" of "what was meant" because the original contained nondeterministic failure cases such as the BDB locks exhaustion business.
Comment by Jacob Welsh — 2022-06-08 @ 15:07
"Schelling point" would be the term I was looking for re "what was meant".
Comment by Jacob Welsh — 2022-06-08 @ 15:21
I meant the BeginMessage()/EndMessage()/AbortMessage() composition, stuff your patches here replace.
Do you mean the CRITICAL_BLOCK() macro? In what ways?
Comment by cgra — 2022-06-08 @ 16:02
Hi cgra, nice of you to drop by and ask some informed questions. I see you've got some articles on your blog about bitcoin code study. While that's well and good, there's nothing there about who you are. The trouble with that being, technology can't, nor can ever, be apolitical. So then, mind sharing either here or, preferably, in some blog articles, just who you might be ? It'll be healthier for everyone. Cheers.
Comment by Robinson Dorion — 2022-06-10 @ 20:35
Oh I see.
No, strictly what I wrote - the behavior of older versions of the code, as a whole, of which the locking is but one facet.
Comment by Jacob Welsh — 2022-06-10 @ 20:54
Hi dorion,
If you find it worthwile, see my chat history. A couple of possibly interesting shortcuts: a micro self-intro, and a mini-review.
In summary and addition, I'm a Finn, with a personal interest in dependable tech (while not implying any kind of proficiency in the field). When I was a kid, dad explained to me how a carpenter may choose to build not the best wooden spoon he can, but one that wears out, or breaks in time -- just to stay in business. I don't know why, but this idea has bothered me ever since. It sounds cheating to me. And of course, the whole world somehow seems to be built on this exact principle.
And I fiddle with bitcoin, because I believe it's better for my future if it succeeds. Or, if not better for myself (say, I blow my chances somehow), I suppose better for the more honest people: perhaps a non-cheating carpenter.
Comment by cgra — 2022-06-11 @ 18:22
Since apparently I didn't previously see the need to fully rub it in regarding the degree of deny-and-blame-others politicking that was at work here in place of anything resembling responsible stewardship:
Comment by Jacob Welsh — 2024-03-20 @ 23:57