Skip to content

Conversation

@theStack
Copy link
Contributor

@theStack theStack commented Feb 6, 2024

Executing the unit tests for the bip324_cipher.py module currently takes quite long (>60 seconds on my older notebook). Most time here is spent in empty plaintext/ciphertext encryption/decryption loops in test_fschacha20poly1305aead:

for _ in range(msg_idx):
enc_aead.encrypt(b"", b"")

for _ in range(msg_idx):
dec_aead.decrypt(b"", bytes(16))

Their sole purpose is increasing the FSChaCha20Poly1305 packet counter in order to trigger rekeying, i.e. the actual encryption/decryption is not relevant, as the result is thrown away. This commit speeds up the tests by supporting to pass "None" as plaintext/ciphertext, indicating to the routines that no actual encryption/decryption should be done.

The approach here is a bit hacky, a cleaner alternative would probably be to introduce a special seek/skip_packets method and not touch the encrypt/decrypt routines, but that seemed overkill to me only for speeding up a unit test. Open for suggestions.

master branch:

$ python3 -m unittest ./test/functional/test_framework/crypto/bip324_cipher.py
..
----------------------------------------------------------------------
Ran 2 tests in 64.658s

PR branch:

$ python3 -m unittest ./test/functional/test_framework/crypto/bip324_cipher.py
..
---------------------------------------------------------------------- 
Ran 2 tests in 0.822s

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 6, 2024

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK epiccurious, marcofleon, cbergqvist, stratospher, achow101
Concept ACK delta1

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@DrahtBot DrahtBot added the Tests label Feb 6, 2024
@theStack theStack changed the title test: speedup bip324_crypto.py unit test test: speedup bip324_cipher.py unit test Feb 6, 2024
Executing the unit tests for the bip324_cipher.py module currently
takes quite long (>60 seconds on my notebook). Most time here is spent
in empty plaintext/ciphertext encryption/decryption loops:

    ....
    for _ in range(msg_idx):
        enc_aead.encrypt(b"", b"")
    ...
    for _ in range(msg_idx):
        enc_aead.decrypt(b"", bytes(16))
    ...

Their sole purpose is increasing the FSChaCha20Poly1305 packet
counters in order to trigger rekeying, i.e. the actual
encryption/decryption is not relevant, as the result is thrown away.
This commit speeds up the tests by supporting to pass "None" as
plaintext/ciphertext, indicating to the routines that no actual
encryption/decryption should be done.

master branch:

$ python3 -m unittest ./test/functional/test_framework/crypto/bip324_cipher.py
..
----------------------------------------------------------------------
Ran 2 tests in 64.658s

PR branch:

$ python3 -m unittest ./test/functional/test_framework/crypto/bip324_cipher.py
..
----------------------------------------------------------------------
Ran 2 tests in 0.822s
@theStack theStack force-pushed the 202402-speedup_bip324_cipher_python_tests branch from 74ef02a to a8c3454 Compare February 6, 2024 00:35
@jo-elimu
Copy link

jo-elimu commented Feb 6, 2024

Is this pull request associated with one of the issues at https://github.com/bitcoin/bitcoin/issues?

@delta1
Copy link

delta1 commented Feb 6, 2024

Concept ACK a8c3454

To run the individual test I had to run it from the test/functional directory this way: python3 -m unittest ./crypto/bip324_cipher.py

Saves about 10 seconds in test_runner on my machine

Before:

Running Unit Tests for Test Framework Modules
.....................
----------------------------------------------------------------------
Ran 21 tests in 15.701s

After:

Running Unit Tests for Test Framework Modules
.....................
----------------------------------------------------------------------
Ran 21 tests in 4.984s

@epiccurious
Copy link
Contributor

a cleaner alternative would probably be to introduce a special seek/skip_packets method and not touch the encrypt/decrypt routines

Why did you choose this approach rather than adding a seek/skip_packet method?

@delta1
Copy link

delta1 commented Feb 6, 2024

Why did you choose this approach rather than adding a seek/skip_packet method?

@epiccurious he explained in the very next line:

that seemed overkill to me only for speeding up a unit test. Open for suggestions

@epiccurious
Copy link
Contributor

Tested ACK a8c3454.

Before (commit 9eeee7c):

Ran 2 tests in 15.541s

After:

Ran 2 tests in 0.204s

Copy link
Contributor

@marcofleon marcofleon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK a8c3454. The comments at the top of bip324_cipher.py specify that this should only be used for testing, so I think this optimization makes sense in that context.

Before:

Ran 2 tests in 8.512s

After:

Ran 2 tests in 0.106s

Copy link
Contributor

@cbergqvist cbergqvist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK a8c3454!

Comment on lines 195 to +196
for _ in range(msg_idx):
enc_aead.encrypt(b"", b"")
enc_aead.encrypt(b"", None)
Copy link
Contributor

@cbergqvist cbergqvist Feb 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding an assert before this loop verifying msg_idx < REKEY_INTERVAL msg_idx > REKEY_INTERVAL to help show intent of the test?

Copy link
Contributor

@stratospher stratospher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK a8c3454. I think it's worth it because of the significant speedup in the unit test.

@theStack, do you get negligible speedup in other v2 tests too? Maybe these negligible benchmarks aren't reliable but I somehow imagined there to be a very negligible slowdown because of an extra check.

  1. unit test went from 10.807s to 0.16s
  2. test/functional/p2p_v2_encrypted.py went from 7.266s to 5.766s
  3. test_runner.py in #29358 went from 1135s to 1125s

@fanquake
Copy link
Member

cc @sipa

@achow101
Copy link
Member

ACK a8c3454

@achow101 achow101 merged commit be5399e into bitcoin:master Feb 29, 2024
kwvg added a commit to kwvg/dash that referenced this pull request Oct 24, 2024
PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Oct 24, 2024
, bitcoin#29372, bitcoin#29460, bitcoin#29358, bitcoin#29511, bitcoin#29390, bitcoin#29431, bitcoin-core/gui#788 (BIP324 backports: part 4)

4735b82 merge bitcoin#29431: disconnection scenarios during v2 handshake (Kittywhiskers Van Gogh)
cc6b88e merge bitcoin-core/gui#788: update session ID tooltip (Kittywhiskers Van Gogh)
2455862 merge bitcoin#29390: speedup bip324_cipher.py unit test (Kittywhiskers Van Gogh)
062aaf1 merge bitcoin#29511: Fix intermittent failure in rpc_net.py --v2transport (Kittywhiskers Van Gogh)
54972e8 merge bitcoin#29358: use v2 everywhere for P2PConnection if --v2transport is enabled (Kittywhiskers Van Gogh)
4cce72f test: add missing debug log assertion in `p2p_invalid_messages.py` (Kittywhiskers Van Gogh)
bd2fe61 merge bitcoin#29460: assert rpc error for addnode v2transport not enabled (Kittywhiskers Van Gogh)
5ee15fa merge bitcoin#29372: fix intermittent failure in `rpc_setban.py --v2transport` (Kittywhiskers Van Gogh)
e278818 merge bitcoin#29352: fix intermittent failure in p2p_v2_earlykeyresponse (Kittywhiskers Van Gogh)
6b2a8b5 merge bitcoin#24748: functional tests for v2 P2P encryption (Kittywhiskers Van Gogh)
32500f2 merge bitcoin#27653: add unit test coverage for Python ECDSA implementation (Kittywhiskers Van Gogh)
9f476c6 net: add Dash network message short IDs, allocate range 128 onwards (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Depends on #6329

  * Dash-specific P2P messages have been allocated short IDs after 128 based on a prior suggestion ([source](#6280 (comment))) as there are 255 potential short IDs (ID `0` is reserved for V1 fallback, [source](https://github.com/dashpay/dash/blob/a7bbcc823d800b293e9ec54dff518fa9929c763c/src/net.cpp#L1019)) and upstream uses 28 short IDs (though Dash has left ID `5` blank as we do not implement the `FEEFILTER` message, [source](https://github.com/dashpay/dash/blob/a7bbcc823d800b293e9ec54dff518fa9929c763c/src/net.cpp#L1024)).

    As it is unlikely that upstream will utilize more than 127 short IDs (and the spec refers to IDs after 32 as "undefined", [source](https://github.com/bitcoin/bips/blob/52894e1aa7af27f4ae7836a391643124c71e2639/bip-0324.mediawiki#v2-bitcoin-p2p-message-structure)), there shouldn't be an adverse effect to utilizing IDs >=128. The unified array of short IDs are accessible through `V2ShortIDs()`.

    * As there are checks to see if an ID *can* be valid by checking against the array size (which wouldn't work here as we create an array of 256 entries combining both upstream and Dash's allocated short IDs, filling the rest with blank values and we cannot ignore blank values to know if a value is unallocated as the blanking could also signal a reservation, [source](https://github.com/dashpay/dash/blob/a7bbcc823d800b293e9ec54dff518fa9929c763c/src/net.cpp#L1048-L1052)), such a check needs to be done by using `IsValidV2ShortID()`.
    * `V2ShortIDs()` isn't as elegant as desired as `std::fill` and `std::copy` are not `constexpr` until C++20 ([source](https://en.cppreference.com/w/cpp/algorithm/fill), [source](https://en.cppreference.com/w/cpp/algorithm/copy)) and until we drop C++17 support, we have to be mindful of that.

  * `masternode connect` will now _attempt_ to establish a P2Pv2 connection if the node *initiating* the connection has opted-in using the new argument (`v2transport`) and the node was started with P2Pv2 enabled (using the launch argument, `-v2transport`).

    This mirrors changes to behavior to `addconnection` introduced in [bitcoin#24748](bitcoin#24748)

  * The oversized payload test in `p2p_invalid_messages.py` will expect an excessively large message of size of `3145729` bytes (and in P2Pv2, `3145742` bytes), as opposed to upstream's `4000001` and `4000014` bytes respectively as Dash has a lower protocol limit of 3MiB ([source](https://github.com/dashpay/dash/blob/a7bbcc823d800b293e9ec54dff518fa9929c763c/src/net.h#L80-L81)) vs Bitcoin's 4MB ([source](https://github.com/bitcoin/bitcoin/blob/225718eda89d65a7041c33e1994d3bf7bd71bbdd/src/net.h#L62-L63))

  ## Breaking Changes

  None expected.

  ## Checklist:

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation **(note: N/A)**
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  PastaPastaPasta:
    utACK 4735b82
  UdjinM6:
    light ACK 4735b82

Tree-SHA512: 4a9d586133633c109e6a8f20a6c6c5e4b24185fb616bcd8568e546b6e9f886e7a8707e811fd070bbe32c40df269f89a56343a24b67242c6147f9df27275af599
@bitcoin bitcoin locked and limited conversation to collaborators Feb 28, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants