highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-11-09 21:47:34 +01:00
Code Issues Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#29575: net_processing: make any misbehavior trigger immediate discouragement

Browse Source 
6eecba475e net_processing: make MaybePunishNodeFor{Block,Tx} return void (Pieter Wuille)
ae60d485da net_processing: remove Misbehavior score and increments (Pieter Wuille)
6457c31197 net_processing: make all Misbehaving increments = 100 (Pieter Wuille)
5120ab1478 net_processing: drop 8 headers threshold for incoming BIP130 (Pieter Wuille)
944c54290d net_processing: drop Misbehavior for unconnecting headers (Pieter Wuille)
9f66ac7cf1 net_processing: do not treat non-connecting headers as response (Pieter Wuille)

Pull request description:

  So far, discouragement of peers triggers when their misbehavior score exceeds 100 points. Most types of misbehavior increment the score by 100, triggering immediate discouragement, but some types do not. This PR makes all increments equal to either 100 (meaning any misbehavior will immediately cause disconnection and discouragement) or 0 (making the behavior effectively unconditionally allowed), and then removes the logic for score accumulation.

  This simplifies the code a bit, but also makes protocol expectations clearer: if a peer misbehaves, they get disconnected. There is no good reason why certain types of protocol violations should be permitted 4 times (howmuch=20) or 9 times (howmuch=10), while many others are never allowed. Furthermore, the distinction between these looks arbitrary.

  The specific types of misbehavior that are changed to 100 are:
  * Sending us a `block` which does not connect to our header tree (which necessarily must have been unsollicited). [used to be score 10]
  * Sending us a `headers` with a non-continuous headers sequence. [used to be score 20]
  * Sending us more than 1000 addresses in a single `addr` or `addrv2` message [used to be score 20]
  * Sending us more than 50000 invs in a single `inv` message [used to be score 20]
  * Sending us more than 2000 headers in a single `headers` message [used to be score 20]

  The specific types of misbehavior that are changed to 0 are:
  * Sending us 10 (*) separate BIP130 headers announcements that do not connect to our block tree [used to be score 20]
  * Sending us more than 8 headers in a single `headers` message (which thus does not get treated as a BIP130 announcement) that does not connect to our block tree. [used to be score 10]

  I believe that none of these behaviors are unavoidable, except for the one marked (*) which can in theory happen still due to interaction between BIP130 and variations in system clocks (the max 2 hour in the future rule). This one has been removed entirely. In order to remove the impact of the bug it was designed to deal with, without relying on misbehavior, a separate improvement is included that makes `getheaders`-tracking more accurate.

  In another unrelated improvement, this also gets rid of the 8 header limit heuristic to determine whether an incoming non-connecting `headers` is a potential BIP130 announcement, as this rule is no longer needed to prevent spurious Misbehavior. Instead, any non-connecting `headers` is now treated as a potential announcement.

ACKs for top commit:
  sr-gi:
    ACK [6eecba4](6eecba475e)
  achow101:
    ACK 6eecba475e
  mzumsande:
    Code Review ACK 6eecba475e
  glozow:
    light code review / concept ACK 6eecba475e

Tree-SHA512: e11e8a652c4ec048d8961086110a3594feefbb821e13f45c14ef81016377be0db44b5311751ef635d6e026def1960aff33f644e78ece11cfb54f2b7daa96f946
This commit is contained in:
Ava Chow
2024-06-20 13:28:38 -04:00
parent aa2ce2d646 6eecba475e
commit a52837b9e9
10 changed files with 100 additions and 170 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
test/functional/p2p_addr_relay.py
View File

@@ -142,7 +142,8 @@ class AddrTest(BitcoinTestFramework):
msg = self.setup_addr_msg(1010)
with self.nodes[0].assert_debug_log(['addr message size = 1010']):
addr_source.send_and_ping(msg)
addr_source.send_message(msg)
addr_source.wait_for_disconnect()
self.nodes[0].disconnect_p2ps()

12
test/functional/p2p_addrv2_relay.py
View File

@@ -86,11 +86,6 @@ class AddrTest(BitcoinTestFramework):
addr_source = self.nodes[0].add_p2p_connection(P2PInterface())
msg = msg_addrv2()
self.log.info('Send too-large addrv2 message')
msg.addrs = ADDRS * 101
with self.nodes[0].assert_debug_log(['addrv2 message size = 1010']):
addr_source.send_and_ping(msg)
self.log.info('Check that addrv2 message content is relayed and added to addrman')
addr_receiver = self.nodes[0].add_p2p_connection(AddrReceiver())
msg.addrs = ADDRS
@@ -106,6 +101,13 @@ class AddrTest(BitcoinTestFramework):
assert addr_receiver.addrv2_received_and_checked
assert_equal(len(self.nodes[0].getnodeaddresses(count=0, network="i2p")), 0)
self.log.info('Send too-large addrv2 message')
msg.addrs = ADDRS * 101
with self.nodes[0].assert_debug_log(['addrv2 message size = 1010']):
addr_source.send_message(msg)
addr_source.wait_for_disconnect()
if __name__ == '__main__':
AddrTest().main()

7
test/functional/p2p_invalid_messages.py
View File

@@ -260,7 +260,9 @@ class InvalidMessagesTest(BitcoinTestFramework):
msg_type = msg.msgtype.decode('ascii')
self.log.info("Test {} message of size {} is logged as misbehaving".format(msg_type, size))
with self.nodes[0].assert_debug_log(['Misbehaving', '{} message size = {}'.format(msg_type, size)]):
self.nodes[0].add_p2p_connection(P2PInterface()).send_and_ping(msg)
conn = self.nodes[0].add_p2p_connection(P2PInterface())
conn.send_message(msg)
conn.wait_for_disconnect()
self.nodes[0].disconnect_p2ps()
def test_oversized_inv_msg(self):
@@ -321,7 +323,8 @@ class InvalidMessagesTest(BitcoinTestFramework):
# delete arbitrary block header somewhere in the middle to break link
del block_headers[random.randrange(1, len(block_headers)-1)]
with self.nodes[0].assert_debug_log(expected_msgs=MISBEHAVING_NONCONTINUOUS_HEADERS_MSGS):
peer.send_and_ping(msg_headers(block_headers))
peer.send_message(msg_headers(block_headers))
peer.wait_for_disconnect()
self.nodes[0].disconnect_p2ps()
def test_resource_exhaustion(self):

9
test/functional/p2p_mutated_blocks.py
View File

@@ -104,11 +104,10 @@ class MutatedBlocksTest(BitcoinTestFramework):
block_missing_prev.hashPrevBlock = 123
block_missing_prev.solve()
# Attacker gets a DoS score of 10, not immediately disconnected, so we do it 10 times to get to 100
for _ in range(10):
assert_equal(len(self.nodes[0].getpeerinfo()), 2)
with self.nodes[0].assert_debug_log(expected_msgs=["AcceptBlock FAILED (prev-blk-not-found)"]):
attacker.send_message(msg_block(block_missing_prev))
# Check that non-connecting block causes disconnect
assert_equal(len(self.nodes[0].getpeerinfo()), 2)
with self.nodes[0].assert_debug_log(expected_msgs=["AcceptBlock FAILED (prev-blk-not-found)"]):
attacker.send_message(msg_block(block_missing_prev))
attacker.wait_for_disconnect(timeout=5)

48
test/functional/p2p_sendheaders.py
View File

@@ -71,19 +71,13 @@ f. Announce 1 more header that builds on that fork.
Expect: no response.
Part 5: Test handling of headers that don't connect.
a. Repeat 10 times:
a. Repeat 100 times:
1. Announce a header that doesn't connect.
Expect: getheaders message
2. Send headers chain.
Expect: getdata for the missing blocks, tip update.
b. Then send 9 more headers that don't connect.
b. Then send 99 more headers that don't connect.
Expect: getheaders message each time.
c. Announce a header that does connect.
Expect: no response.
d. Announce 49 headers that don't connect.
Expect: getheaders message each time.
e. Announce one more that doesn't connect.
Expect: disconnect.
"""
from test_framework.blocktools import create_block, create_coinbase
from test_framework.messages import CInv
@@ -526,7 +520,8 @@ class SendHeadersTest(BitcoinTestFramework):
# First we test that receipt of an unconnecting header doesn't prevent
# chain sync.
expected_hash = tip
for i in range(10):
NUM_HEADERS = 100
for i in range(NUM_HEADERS):
self.log.debug("Part 5.{}: starting...".format(i))
test_node.last_message.pop("getdata", None)
blocks = []
@@ -550,41 +545,24 @@ class SendHeadersTest(BitcoinTestFramework):
blocks = []
# Now we test that if we repeatedly don't send connecting headers, we
# don't go into an infinite loop trying to get them to connect.
MAX_NUM_UNCONNECTING_HEADERS_MSGS = 10
for _ in range(MAX_NUM_UNCONNECTING_HEADERS_MSGS + 1):
for _ in range(NUM_HEADERS + 1):
blocks.append(create_block(tip, create_coinbase(height), block_time))
blocks[-1].solve()
tip = blocks[-1].sha256
block_time += 1
height += 1
for i in range(1, MAX_NUM_UNCONNECTING_HEADERS_MSGS):
# Send a header that doesn't connect, check that we get a getheaders.
for i in range(1, NUM_HEADERS):
with p2p_lock:
test_node.last_message.pop("getheaders", None)
# Send an empty header as a failed response to the received getheaders
# (from the previous iteration). Otherwise, the new headers will be
# treated as a response instead of as an announcement.
test_node.send_header_for_blocks([])
# Send the actual unconnecting header, which should trigger a new getheaders.
test_node.send_header_for_blocks([blocks[i]])
test_node.wait_for_getheaders(block_hash=expected_hash)
# Next header will connect, should re-set our count:
test_node.send_header_for_blocks([blocks[0]])
expected_hash = blocks[0].sha256
# Remove the first two entries (blocks[1] would connect):
blocks = blocks[2:]
# Now try to see how many unconnecting headers we can send
# before we get disconnected. Should be 5*MAX_NUM_UNCONNECTING_HEADERS_MSGS
for i in range(5 * MAX_NUM_UNCONNECTING_HEADERS_MSGS - 1):
# Send a header that doesn't connect, check that we get a getheaders.
test_node.send_header_for_blocks([blocks[i % len(blocks)]])
test_node.wait_for_getheaders(block_hash=expected_hash)
# Eventually this stops working.
test_node.send_header_for_blocks([blocks[-1]])
# Should get disconnected
test_node.wait_for_disconnect()
self.log.info("Part 5: success!")
# Finally, check that the inv node never received a getdata request,
# throughout the test
assert "getdata" not in inv_node.last_message

4
test/functional/p2p_unrequested_blocks.py
View File

@@ -170,9 +170,11 @@ class AcceptBlockTest(BitcoinTestFramework):
tip = next_block
# Now send the block at height 5 and check that it wasn't accepted (missing header)
test_node.send_and_ping(msg_block(all_blocks[1]))
test_node.send_message(msg_block(all_blocks[1]))
test_node.wait_for_disconnect()
assert_raises_rpc_error(-5, "Block not found", self.nodes[0].getblock, all_blocks[1].hash)
assert_raises_rpc_error(-5, "Block not found", self.nodes[0].getblockheader, all_blocks[1].hash)
test_node = self.nodes[0].add_p2p_connection(P2PInterface())
# The block at height 5 should be accepted if we provide the missing header, though
headers_message = msg_headers()