The messages.py module should contain code and helpers for
[de]serializing p2p messages. Specific usage of those messages should
be in p2p.py. Therefore specify the nServices value in the calling code,
not in the messages.py module.
messages.py is for message and primitive data structures. Specifics
about the test framework's p2p implementation should be in p2p.py.
Also rename to P2P_VERSION_RELAY. Also rename msg_version.nRelay to
relay. In Bitcoin Core, this is referred to as fRelay, since it's a
bool, so this field has always been misnamed.
The messages.py module should contain code and helpers for
[de]serializing p2p messages. Specific usage of those messages should
be in p2p.py. Therefore move MY_SUBVERSION to p2p.py.
Also rename to P2P_SUBVERSION.
The messages.py module should contain code and helpers for
[de]serializing p2p messages. Specific usage of those messages should
be in p2p.py. Therefore move MY_VERSION to p2p.py.
Also rename to P2P_VERSION to distinguish it from
other versioning used in Bitcoin/Bitcoin Core.
Also always set the nVersion field in CBlockLocator to 0 and ignore the
field in deserialized messages. The field is not currently used for
anything in Bitcoin Core.
The messages.py module should contain code and helpers for
[de]serializing p2p messages. Specific usage of those messages should
be in p2p.py. Therefore move MIN_VERSION_SUPPORTED to p2p.py.
Also rename to MIN_P2P_VERSION_SUPPORTED to distinguish it from
other versioning used in Bitcoin/Bitcoin Core.
de85af5cce test: store subversion (user agent) as string in msg_version (Sebastian Falbesoner)
Pull request description:
It seems more natural to treat the "subversion" field (=user agent string, see [BIP 14](https://github.com/bitcoin/bips/blob/master/bip-0014.mediawiki#Proposal)) of a node as pure string rather than a bytestring within the test framework. This is also suggested with the naming prefix in `msg_version.strSubVer`: one probably wouldn't expect a field starting with "str" to be a bytestring that needs further decoding to be useful. This PR moves the encoding/decoding parts to the serialization/deserialization routines so that the user doesn't have to bother with that anymore.
Note that currently, in the master branch the `msg_version.strSubVer` is never read (only in `msg_version.__repr__`); However, one issue that is solved by this PR came up while testing #19509 (not merged yet): A decoding script for binary message capture files takes use of the functional test framework convert it into JSON format. Bytestrings will be convered to hexstrings, while pure strings will (surprise surprise) end up without modification in the file.
So without this patch, we get:
```
$ jq . out.json | grep -m5 strSubVer
"strSubVer": "2f5361746f7368693a32312e39392e302f"
"strSubVer": "2f5361746f7368693a302e32302e312f"
"strSubVer": "2f5361746f7368693a32312e39392e302f"
"strSubVer": "2f5361746f7368693a302e32302e312f"
"strSubVer": "2f5361746f7368693a32312e39392e302f"
```
After this patch:
```
$ jq . out2.json | grep -m5 strSubVer
"strSubVer": "/Satoshi:21.99.0/"
"strSubVer": "/Satoshi:0.20.1/"
"strSubVer": "/Satoshi:21.99.0/"
"strSubVer": "/Satoshi:0.20.1/"
"strSubVer": "/Satoshi:21.99.0/"
```
ACKs for top commit:
jnewbery:
utACK de85af5cce
Tree-SHA512: ff23642705c858e8387a625537dfec82e6b8a15da6d99b8d12152560e52d243ba17431b602b26f60996d897e00e3f37dcf8dc8a303ffb1d544df29a5937080f9
This commit adds contrib/message-capture/message-capture-parser.py, a python
script to be used alongside -capturemessages to parse the captured
messages.
It is complete with arguments and will parse any file given, sorting the
messages in the files when creating the output. If an output file is
specified with -o or --output, it will dump the messages in json format
to that file, otherwise it will print to stdout.
The small change to the unused msg_generic is to bring it in line with
the other message classes, purely to avoid a bug in the future.
Introduce a new message `sendaddrv2` to signal support for ADDRv2.
Send the new message immediately after sending the `VERACK` message.
Add support for receiving and parsing ADDRv2 messages.
Send ADDRv2 messages (instead of ADDR) to a peer if he has
advertised support for it.
Co-authored-by: Carl Dong <contact@carldong.me>
638441928a test: add parameterized constructor for msg_sendcmpct() (Sebastian Falbesoner)
Pull request description:
While working on the test for #19776 I noticed that creating a `sendcmpct` message is quite cumbersome -- due to the lack of a parameterized constructor, one needs to create an empty (that is, initialized with default values) object and then set the two fields one by one. This PR replaces the default constructor with a parameterized constructor and uses it in the test `p2p_compactblocks.py`, reducing LOC. No need to pollute the namespace with temporary throw-away message objects anymore.
ACKs for top commit:
guggero:
Code review ACK 638441928a.
epson121:
Code review ACK 638441928a
Tree-SHA512: 3b58d276d714b73abc6cc98d1d52dec5f6026b33f03faaeb7dcbc5d83ac377555179f98b159b2b9ecc8957999c35a1dc082e3c69299c5fde4e35f1bd0587ce9d
fb56d37612 p2p: ensure inv is GenMsgTx before ToGenTxid in inv processing (John Newbery)
aa3621385e test: use CInv::MSG_WITNESS_TX flag in p2p_segwit (Jon Atack)
24ee4f01ea p2p: make gtxid(.hash) and fAlreadyHave localvars const (Jon Atack)
b1c855453b p2p: use CInv block message helpers in net_processing.cpp (Jon Atack)
acd6642167 [net processing] Change AlreadyHaveTx() to take a GenTxid (John Newbery)
5fdfb80b86 [net processing] Change AlreadyHaveBlock() to take block_hash argument (John Newbery)
430e183b89 [net processing] Remove mempool argument from AlreadyHaveBlock() (John Newbery)
42ca5618ca [net processing] Split AlreadyHave() into separate block and tx functions (John Newbery)
39f1dc9445 p2p: remove nFetchFlags from NetMsgType TX and INV processing (Jon Atack)
471714e1f0 p2p: add CInv block message helper methods (Jon Atack)
Pull request description:
Building on #19590 and the recent `wtxid` and `GenTxid` changes, this is a refactoring and cleanup PR to simplify and improve some of the net processing code.
Some of the diffs are best reviewed with `-w` to ignore spacing.
Co-authored by John Newbery.
ACKs for top commit:
laanwj:
Code review ACK fb56d37612
jnewbery:
utACK fb56d37612
vasild:
ACK fb56d3761
Tree-SHA512: ba39b58e6aaf850880a842fe5f6295e9f1870906ef690206acfc17140aae2ac854981e1066dbcd4238062478762fbd040ef772fdc2c50eea6869997c583e6a6d
f5c003d3ea [test] Add test for NODE_COMPACT_FILTER. (Jim Posen)
132b30d9c8 [net] Signal NODE_COMPACT_FILTERS if we're serving compact filters. (Jim Posen)
b3fbc94d4f Apply cfilters review fixups (John Newbery)
Pull request description:
If -peerblockfilters is configured, signal the `NODE_COMPACT_FILTERS` service bit to indicate that we are able to serve compact block filters, headers and checkpoints.
ACKs for top commit:
MarcoFalke:
re-review and Concept ACK f5c003d3ea
fjahr:
Code review ACK f5c003d3ea
clarkmoody:
Concept ACK f5c003d3ea
ariard:
Concept and Code Review ACK f5c003d
jonatack:
ACK f5c003d3e
Tree-SHA512: 34d1c153530a0e55d09046fe548c9dc37344b5d6d50e00af1b4e1de1e7b49de770fca8471346a17c151de9fe164776296bb3dd5af331977f0c3ef1e6fc906f85
substitutes "for x in range(N):" by "for _ in range(N):"
indicates to the reader that a block is just repeated N times, and
that the loop counter is not used in the body
4a614ff88a test: explicit imports from test_framework.messages in p2p_invalid_messages.py (Sebastian Falbesoner)
b35e1d2471 test: add inventory type constant MSG_CMPCT_BLOCK (Sebastian Falbesoner)
eeaaa58d2c test: replace inv type magic numbers by constants (Sebastian Falbesoner)
Pull request description:
Many functional tests still use magic numbers for inventory types, either passed to the `CInv` constructor or for comparing the `type` member of `CInv`. This PR replaces all of those by constants in the module `test_framework.messages` that have been introduced in commit c32cf9f622: `MSG_TX` (1) or `MSG_BLOCK` (2).
It also introduces a new constant `MSG_CMPCT_BLOCK` (naming as in `src/protocol.h`) and uses it to replace the remaining magic numbers.
The occurences of the magic numbers were identified through `grep`ing for `CInv(` and `type ==`. The idea was first to create a scripted-diff, but since also adding missing `import`s is needed, this would be non-trivial. Besides, also some unneeded comments like `# 2 == "Block"` could be removed.
ACKs for top commit:
gzhao408:
ACK [`4a614ff`](4a614ff88a)
Tree-SHA512: 4ba4fdef9f3eef7fd5ac72cb03ca3524863d1ae292161c550424a4c1047283fa2d2e7e03017d1fbae3652b3cb14f08b8d4b368403f3f209993aef3f2e2b22784
as defined in PEP 3135:
"The new syntax:
super()
is equivalent to:
super(__class__, <firstarg>)
where __class__ is the class that the method was defined in, and <firstarg> is
the first parameter of the method (normally self for instance methods, and cls
for class methods)."
9df32e820d scripted-diff: test: replace command with msgtype (Sebastian Falbesoner)
Pull request description:
This is a follow-up PR to https://github.com/bitcoin/bitcoin/pull/18533, which changed the naming of `strCommand` to `msg_type` in the network processing code. The same approach is done here for the function test framework, to get rid of the wrong "command" terminology for network mesage types. (Commands are usually used in the CLI or RPC context, so using the same name in the network message context would only be confusing.)
The commit was created through the following steps:
1. search for all occurences of the string "command" within the folder `test/functional`
```git grep -i command test/functional > command_finds```
2. manually sort out all false-positives, i.e. occurences of "command" which describe commands in the correct sense (mostly CLI or RPC related, also some with Socks5)
3. put the remaining occurences into a scripted-diff (a quite simple one, actually) that renames "command" to "msgtype" in the concerned files.
The name `msgtype` was intentionally chosen without the underscore `_` as classes beginning with `msg_` define concrete types of messages.
ACKs for top commit:
MarcoFalke:
ACK 9df32e820d . Makes sense that tests use the same naming as Bitcoin Core. See `NetMsgType` here: https://doxygen.bitcoincore.org/namespace_net_msg_type.html
Tree-SHA512: cd0ee08a382910b7f10ce583acdaf4f8a39f9ba4a22434a914415727eedd98bac538de9bf6633574d5eb86f62558bc8dcb638a3289d99b04f8481f34e7a9a0c7
This is the functional test framework pendant for
7777e3624f, which renamed "strCommand" with
"msg_type" in the network processing code.
-BEGIN VERIFY SCRIPT-
# Rename in test framework
sed -i 's/command/msgtype/g' ./test/functional/test_framework/messages.py ./test/functional/test_framework/mininode.py
# Rename in individual tests
sed -i 's/command/msgtype/g' ./test/functional/p2p_invalid_messages.py ./test/functional/p2p_leak.py
-END VERIFY SCRIPT-
Implements the missing initialization/serialization methods for
msg_merkleblock, based on the already present class CMerkleBlock.
Also changes the method wait_for_merkleblock() to be more precise by waiting
for a merkleblock with a specified blockhash instead of an arbitrary one.
In the BIP37 test p2p_filter.py, this new method is used to make the test of
receiving merkleblock and tx if a filter is set to be more precise, by checking
if they also arrive in the right order.
bead32e31e Add release notes for DEFAULT_BLOOM change (Matt Corallo)
f27309f55c Move DEFAULT_PEERBLOOMFILTERS from validation.h to net_processing.h (Matt Corallo)
5efcb77283 Disable bloom filtering by default. (Matt Corallo)
Pull request description:
BIP 37 bloom filters have been well-known to be a significant DoS
target for some time. However, in order to provide continuity for
SPV clients relying on it, the NODE_BLOOM service flag was added,
and left as a default, to ensure sufficient nodes exist with such a
flag.
NODE_BLOOM is, at this point, well-established and, as long as
there exist 0.18 nodes with default config (which I'd anticipate
will be true for many years), will be available from some peers. By
that time, the continued slowdown of BIP 37-based filtering will
likely have rendered it useless (though this is already largely the
case). Further, BIP 37 was deliberately never updated to support
witness-based filtering as newer wallets are expected to migrate to
some yet-to-be-network-exposed filters.
ACKs for top commit:
jnewbery:
ACK bead32e31e
kallewoof:
ACK bead32e31e
Tree-SHA512: ecd901898e8efe1a7c82b471af0acc2373c2282ac633eb58d9aae7c35deda1999d0f79fb0485e6cecbda7246aeda00206cd82c7fa36866e2ac64705ba93f9390
BIP 37 bloom filters have been well-known to be a significant DoS
target for some time. However, in order to provide continuity for
SPV clients relying on it, the NODE_BLOOM service flag was added,
and left as a default, to ensure sufficient nodes exist with such a
flag.
NODE_BLOOM is, at this point, well-established and, as long as
there exist 0.18 nodes with default config (which I'd anticipate
will be true for many years), will be available from some peers. By
that time, the continued slowdown of BIP 37-based filtering will
likely have rendered it useless (though this is already largely the
case). Further, BIP 37 was deliberately never updated to support
witness-based filtering as newer wallets are expected to migrate to
some yet-to-be-network-exposed filters.
This diff has been generated with the following script, but is better
reviewed without looking at the script.
# -BEGIN VERIFY SCRIPT-
echo "Use msg_witness_block everywhere, except for tests that require msg_block"
# This could be a separate commit, but it is combined with the
# following scripts to reduce the overall diff
sed -i -e 's/msg_block/msg_witness_block/g' ./test/functional/{feature_assumevalid,feature_cltv,feature_dersig,feature_versionbits_warning,p2p_fingerprint,p2p_sendheaders,p2p_unrequested_blocks,example_test,rpc_blockchain}.py
echo "Rename msg_block to msg_no_witness_block"
# Rename msg_block to msg_no_witness_block in all tests (not the
# framework)
sed -i -e 's/msg_block/msg_no_witness_block/g' $(git grep -l msg_block ./test/functional/*.py)
# Derive msg_no_witness_block from msg_block
# Make msg_block a witness block in messages.py
patch -p1 --fuzz 0 << EOF
diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py
index 00190e4cbd..e454ed5987 100755
--- a/test/functional/test_framework/messages.py
+++ b/test/functional/test_framework/messages.py
@@ -1133 +1133 @@ class msg_block:
- return self.block.serialize(with_witness=False)
+ return self.block.serialize()
@@ -1155 +1155 @@ class msg_generic:
-class msg_witness_block(msg_block):
+class msg_no_witness_block(msg_block):
@@ -1158,2 +1158 @@ class msg_witness_block(msg_block):
- r = self.block.serialize()
- return r
+ return self.block.serialize(with_witness=False)
@@ -1445 +1444 @@ class msg_blocktxn:
- r += self.block_transactions.serialize(with_witness=False)
+ r += self.block_transactions.serialize()
@@ -1452 +1451 @@ class msg_blocktxn:
-class msg_witness_blocktxn(msg_blocktxn):
+class msg_no_witness_blocktxn(msg_blocktxn):
@@ -1456,3 +1455 @@ class msg_witness_blocktxn(msg_blocktxn):
- r = b""
- r += self.block_transactions.serialize()
- return r
+ return self.block_transactions.serialize(with_witness=False)
EOF
# Conclude rename of msg_block to msg_no_witness_block
sed -i -e 's/msg_witness_block/msg_block/g' $(git grep -l msg_witness_block)
# -END VERIFY SCRIPT-
Unnamed arguments are confusing as to what they mean without looking up
the function signature.
Since segwit is active by default in regtest, and all blocks are
serialized with witness (#15664, c459c5f), remove the argument
`with_witness=True` from all calls to `CBlock::serialize` and
`BlockTransactions::serialize`.
This diff has been created with a script, but is better reviewed without
a scripted diff.
sed -i --regexp-extended -e 's/block(_?[2a-z]*)\.serialize\([a-z_]*=?True/block\1.serialize(/g' $(git grep -l serialize ./test)
