Skip to content

Commit

Permalink
net: Add option -enablebip61 to configure sending of BIP61 notifica…
Browse files Browse the repository at this point in the history 
…tions

This commit adds a boolean option `-enablebip61`, defaulting to `1`, that
can be used to disable the sending of BIP61 `reject` messages. This
functionality has been requested for various reasons:

- security (DoS): reject messages can reveal internal state that can be
  used to target certain resources such as the mempool more easily.

- bandwidth: a typical node sends lots of reject messages; this counts
  against upstream bandwidth. Also the reject messages tend to be larger
  than the message that was rejected.

On the other hand, reject messages can be useful while developing client
software (I found them indispensable while creating bitcoin-submittx),
as well as for our own test cases, so whatever the default becomes on the
long run, IMO the functionality should be retained as option. But that's
a discussion for later.
  • Loading branch information
@laanwj
laanwj committed May 13, 2018
1 parent ffa86af commit fe16dd8
Show file tree
Hide file tree
Showing 4 changed files with 39 additions and 10 deletions.
3 changes: 3 additions & 0 deletions src/init.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,7 @@ void SetupServerArgs()
gArgs.AddArg("-discover", _("Discover own IP addresses (default: 1 when listening and no -externalip or -proxy)"), false, OptionsCategory::CONNECTION);
gArgs.AddArg("-dns", _("Allow DNS lookups for -addnode, -seednode and -connect") + " " + strprintf(_("(default: %u)"), DEFAULT_NAME_LOOKUP), false, OptionsCategory::CONNECTION);
gArgs.AddArg("-dnsseed", _("Query for peer addresses via DNS lookup, if low on addresses (default: 1 unless -connect used)"), false, OptionsCategory::CONNECTION);
gArgs.AddArg("-enablebip61", strprintf(_("Send reject messages per BIP61 (default: %u)"), DEFAULT_ENABLE_BIP61), false, OptionsCategory::CONNECTION);
gArgs.AddArg("-externalip=<ip>", _("Specify your own public address"), false, OptionsCategory::CONNECTION);
gArgs.AddArg("-forcednsseed", strprintf(_("Always query for peer addresses via DNS lookup (default: %u)"), DEFAULT_FORCEDNSSEED), false, OptionsCategory::CONNECTION);
gArgs.AddArg("-listen", _("Accept connections from outside (default: 1 if no -proxy or -connect)"), false, OptionsCategory::CONNECTION);
Expand Down Expand Up @@ -1099,6 +1100,8 @@ bool AppInitParameterInteraction()
if (gArgs.GetBoolArg("-peerbloomfilters", DEFAULT_PEERBLOOMFILTERS))
nLocalServices = ServiceFlags(nLocalServices | NODE_BLOOM);

g_enable_bip61 = gArgs.GetBoolArg("-enablebip61", DEFAULT_ENABLE_BIP61);

if (gArgs.GetArg("-rpcserialversion", DEFAULT_RPC_SERIALIZE_VERSION) < 0)
return InitError("rpcserialversion must be non-negative.");

Expand Down
30 changes: 21 additions & 9 deletions src/net_processing.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
#endif

std::atomic<int64_t> nTimeBestReceived(0); // Used only to inform the wallet of when we last received a block
bool g_enable_bip61 = DEFAULT_ENABLE_BIP61;

struct IteratorComparator
{
Expand Down Expand Up @@ -1579,7 +1580,9 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
// Each connection can only send one version message
if (pfrom->nVersion != 0)
{
connman->PushMessage(pfrom, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::REJECT, strCommand, REJECT_DUPLICATE, std::string("Duplicate version message")));
if (g_enable_bip61) {
connman->PushMessage(pfrom, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::REJECT, strCommand, REJECT_DUPLICATE, std::string("Duplicate version message")));
}
LOCK(cs_main);
Misbehaving(pfrom->GetId(), 1);
return false;
Expand Down Expand Up @@ -1608,8 +1611,10 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
if (!pfrom->fInbound && !pfrom->fFeeler && !pfrom->m_manual_connection && !HasAllDesirableServiceFlags(nServices))
{
LogPrint(BCLog::NET, "peer=%d does not offer the expected services (%08x offered, %08x expected); disconnecting\n", pfrom->GetId(), nServices, GetDesirableServiceFlags(nServices));
connman->PushMessage(pfrom, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::REJECT, strCommand, REJECT_NONSTANDARD,
strprintf("Expected to offer services %08x", GetDesirableServiceFlags(nServices))));
if (g_enable_bip61) {
connman->PushMessage(pfrom, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::REJECT, strCommand, REJECT_NONSTANDARD,
strprintf("Expected to offer services %08x", GetDesirableServiceFlags(nServices))));
}
pfrom->fDisconnect = true;
return false;
}
Expand All @@ -1629,8 +1634,10 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
{
// disconnect from peers older than this proto version
LogPrint(BCLog::NET, "peer=%d using obsolete version %i; disconnecting\n", pfrom->GetId(), nVersion);
connman->PushMessage(pfrom, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::REJECT, strCommand, REJECT_OBSOLETE,
strprintf("Version must be %d or greater", MIN_PEER_PROTO_VERSION)));
if (g_enable_bip61) {
connman->PushMessage(pfrom, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::REJECT, strCommand, REJECT_OBSOLETE,
strprintf("Version must be %d or greater", MIN_PEER_PROTO_VERSION)));
}
pfrom->fDisconnect = true;
return false;
}
Expand Down Expand Up @@ -2328,9 +2335,10 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
LogPrint(BCLog::MEMPOOLREJ, "%s from peer=%d was not accepted: %s\n", tx.GetHash().ToString(),
pfrom->GetId(),
FormatStateMessage(state));
if (state.GetRejectCode() > 0 && state.GetRejectCode() < REJECT_INTERNAL) // Never send AcceptToMemoryPool's internal codes over P2P
if (g_enable_bip61 && state.GetRejectCode() > 0 && state.GetRejectCode() < REJECT_INTERNAL) { // Never send AcceptToMemoryPool's internal codes over P2P
connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::REJECT, strCommand, (unsigned char)state.GetRejectCode(),
state.GetRejectReason().substr(0, MAX_REJECT_MESSAGE_LENGTH), inv.hash));
}
if (nDoS > 0) {
Misbehaving(pfrom->GetId(), nDoS);
}
Expand Down Expand Up @@ -2903,8 +2911,10 @@ static bool SendRejectsAndCheckIfBanned(CNode* pnode, CConnman* connman)
AssertLockHeld(cs_main);
CNodeState &state = *State(pnode->GetId());

for (const CBlockReject& reject : state.rejects) {
connman->PushMessage(pnode, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::REJECT, std::string(NetMsgType::BLOCK), reject.chRejectCode, reject.strRejectReason, reject.hashBlock));
if (g_enable_bip61) {
for (const CBlockReject& reject : state.rejects) {
connman->PushMessage(pnode, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::REJECT, std::string(NetMsgType::BLOCK), reject.chRejectCode, reject.strRejectReason, reject.hashBlock));
}
}
state.rejects.clear();

Expand Down Expand Up @@ -3011,7 +3021,9 @@ bool PeerLogicValidation::ProcessMessages(CNode* pfrom, std::atomic<bool>& inter
}
catch (const std::ios_base::failure& e)
{
connman->PushMessage(pfrom, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::REJECT, strCommand, REJECT_MALFORMED, std::string("error parsing message")));
if (g_enable_bip61) {
connman->PushMessage(pfrom, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::REJECT, strCommand, REJECT_MALFORMED, std::string("error parsing message")));
}
if (strstr(e.what(), "end of data"))
{
// Allow exceptions from under-length message on vRecv
Expand Down
5 changes: 5 additions & 0 deletions src/net_processing.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,11 @@ static constexpr int64_t EXTRA_PEER_CHECK_INTERVAL = 45;
/** Minimum time an outbound-peer-eviction candidate must be connected for, in order to evict, in seconds */
static constexpr int64_t MINIMUM_CONNECT_TIME = 30;

/** Default for BIP61 (sending reject messages) */
static constexpr bool DEFAULT_ENABLE_BIP61 = true;
/** Enable BIP61 (sending reject messages) */
extern bool g_enable_bip61;

class PeerLogicValidation final : public CValidationInterface, public NetEventsInterface {
private:
CConnman* const connman;
Expand Down
11 changes: 10 additions & 1 deletion test/functional/p2p_invalid_tx.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
)


REJECT_INVALID = 16

class InvalidTxRequestTest(BitcoinTestFramework):
def set_test_params(self):
self.num_nodes = 1
Expand Down Expand Up @@ -71,7 +73,7 @@ def run_test(self):
# and we get disconnected immediately
self.log.info('Test a transaction that is rejected')
tx1 = create_transaction(block1.vtx[0], 0, b'\x64' * 35, 50 * COIN - 12000)
node.p2p.send_txs_and_test([tx1], node, success=False, expect_disconnect=True)
node.p2p.send_txs_and_test([tx1], node, success=False, expect_disconnect=True, reject_code=REJECT_INVALID, reject_reason=b'mandatory-script-verify-flag-failed (Invalid OP_IF construction)')

# Make two p2p connections to provide the node with orphans
# * p2ps[0] will send valid orphan txs (one with low fee)
Expand Down Expand Up @@ -137,6 +139,13 @@ def run_test(self):
wait_until(lambda: 1 == len(node.getpeerinfo()), timeout=12) # p2ps[1] is no longer connected
assert_equal(expected_mempool, set(node.getrawmempool()))

# restart node with sending BIP61 messages disabled, check that it disconnects without sending the reject message
self.log.info('Test a transaction that is rejected, with BIP61 disabled')
self.restart_node(0, ['-enablebip61=0','-persistmempool=0'])
self.reconnect_p2p(num_connections=1)
node.p2p.send_txs_and_test([tx1], node, success=False, expect_disconnect=True)
# send_txs_and_test will have waited for disconnect, so we can safely check that no reject has been received
assert_equal(node.p2p.reject_code_received, None)

if __name__ == '__main__':
InvalidTxRequestTest().main()

0 comments on commit fe16dd8

Please sign in to comment.