Motivation
This PR extends the testing framework to add a BIP324 enabled node so v2 connections can be tested.
This made me check BIP324 more in depth: BIP324
Thoughts/Questions
- Why do we need
queue_messages? Feels like only a single message may be queued.
- Turns out this was not needed, as pointed out
- I wonder why, in
hkdf.py , hkdf_sha256 was defined as a mix of HKDF_Extract and HKDF_Expand. It it stated in the file that it is designed to ease understanding and not performance. However, splitting this seems pretty trivial and avoids having to rehash every time hkdf_sha256 is called, given prk is always the same.
- I’ve benchmarked this and looks like its not a big issue, so I’m not even mentioning it
- I think we are not properly testing connection downcasting due to some bytes of the v1 matching but not all. Debugging the tests I only see the connection being identified at the first sent byte, but not after.
- Turns out we are, but not on both sides, which happens to make sense: the test node is not testing this agains the Core node given the Core node does not do this, and there is no way to test the test node. So the test node either downcast after the last byte or does not at all
In general I think this looks good, I don’t think its worth going into nitpicking hell/python idiomatic and styling being this only part of the test suite and given it does test what it is supposed to.
Review
First pass
test/BIP324: functional tests for v2 P2P encryption by stratospher · Pull Request #24748 · bitcoin/bitcoin
Second pass
test/BIP324: functional tests for v2 P2P encryption by stratospher · Pull Request #24748 · bitcoin/bitcoin