Make PeerManager own a FastRandomContext #28558

dergoegge commented at 1:48 PM on October 2, 2023: member

This lets us avoid some non-determinism in tests (also see #28537).

DrahtBot commented at 1:48 PM on October 2, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	MarcoFalke, glozow

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28043 (fuzz: Test headers pre-sync through p2p interface by dergoegge)
#27826 (validation: log which peer sent us a header by Sjors)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

dergoegge commented at 1:48 PM on October 2, 2023: member

cc @MarcoFalke

in src/net_processing.cpp:698 in 485dc89553 outdated

 694 | @@ -695,6 +695,10 @@ class PeerManagerImpl final : public PeerManager
 695 |      /** Send `feefilter` message. */
 696 |      void MaybeSendFeefilter(CNode& node, Peer& peer, std::chrono::microseconds current_time) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex);
 697 |  
 698 | +    FastRandomContext m_rng;

maflcko commented at 3:03 PM on October 2, 2023:

FastRandomContext is not thread safe, iirc? If so, could add the lock annotation?

maflcko commented at 3:05 PM on October 2, 2023:

Also, could add a test-only method to (re)set the state?

dergoegge commented at 10:44 AM on October 3, 2023:

Added the annotations.

dergoegge commented at 10:46 AM on October 3, 2023:

Also, could add a test-only method to (re)set the state?

There's a lot of state in peerman that needs resetting and I'd prefer if we'd just make the test setup initialization faster so we can setup a new peerman each iteration.

maflcko commented at 3:58 PM on October 5, 2023:

Also, could add a test-only method to (re)set the state?

There's a lot of state in peerman that needs resetting and I'd prefer if we'd just make the test setup initialization faster so we can setup a new peerman each iteration.

Are you going to work on that?

DrahtBot added the label CI failed on Oct 2, 2023

[net processing] PeerManager holds a FastRandomContext 87c706713e

[net processing] PushAddress uses PeerManager's rng a648dd79e5

[net processing] Addr shuffle uses PeerManager's rng 77506f4ac6

dergoegge force-pushed on Oct 3, 2023

in src/test/fuzz/fees.cpp:20 in 209cd6ad19 outdated

  16 | @@ -17,7 +17,8 @@ FUZZ_TARGET(fees)
  17 |  {
  18 |      FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
  19 |      const CFeeRate minimal_incremental_fee{ConsumeMoney(fuzzed_data_provider)};
  20 | -    FeeFilterRounder fee_filter_rounder{minimal_incremental_fee};
  21 | +    FastRandomContext rng;

maflcko commented at 10:37 AM on October 3, 2023:

What is the point of this pull request, when you keep the FRC to be random in fuzz tests?

dergoegge commented at 10:43 AM on October 3, 2023:

fixed

dergoegge force-pushed on Oct 3, 2023

DrahtBot removed the label CI failed on Oct 4, 2023

dergoegge commented at 10:13 AM on October 4, 2023: member

Can someone restart the win64 job? failure is unrelated

in src/net_processing.cpp:5341 in 32a42af8b1 outdated

5337 | @@ -5338,7 +5338,7 @@ void PeerManagerImpl::MaybeSendFeefilter(CNode& pto, Peer& peer, std::chrono::mi
5338 |      if (pto.IsBlockOnlyConn()) return;
5339 |  
5340 |      CAmount currentFilter = m_mempool.GetMinFee().GetFeePerK();
5341 | -    static FeeFilterRounder g_filter_rounder{CFeeRate{DEFAULT_MIN_RELAY_TX_FEE}};
5342 | +    static FeeFilterRounder g_filter_rounder{CFeeRate{DEFAULT_MIN_RELAY_TX_FEE}, m_rng};

maflcko commented at 11:14 AM on October 4, 2023:

You can't have a static lifetime object that has a reference to a non-static member field.

This should fail the tests :(

dergoegge commented at 11:50 AM on October 4, 2023:

Right, I can shuffle the commits to de-globalize before passing in the rng

dergoegge commented at 12:17 PM on October 4, 2023:

Done

in src/test/policy_fee_tests.cpp:16 in 32a42af8b1 outdated

  12 | @@ -13,7 +13,8 @@ BOOST_AUTO_TEST_SUITE(policy_fee_tests)
  13 |  
  14 |  BOOST_AUTO_TEST_CASE(FeeRounder)
  15 |  {
  16 | -    FeeFilterRounder fee_rounder{CFeeRate{1000}};
  17 | +    FastRandomContext rng;

maflcko commented at 11:15 AM on October 4, 2023:

Missing deterministic arg for clarity?

dergoegge commented at 12:17 PM on October 4, 2023:

Done

maflcko approved

maflcko commented at 11:16 AM on October 4, 2023: member

lgtm ACK 7022d6a93db08b1334ddae9df439186308bfdd4b 🎇

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 7022d6a93db08b1334ddae9df439186308bfdd4b 🎇
Vvrl6j+oChrlRcQJFM+xw385dp6qeuK8yw7FZZj2uY0iIWAtdofcaOsmCdXTS0KPV8XWyQzkVw94xJeYmx4GBQ==

</details>

[net processing] Make fee filter rounder non-global 47520ed209

[net processing] FeeFilterRounder doesn't own a FastRandomContext fecec3e1c6

[test] Make PeerManager's rng deterministic in tests 4cafe9f176

dergoegge force-pushed on Oct 4, 2023

maflcko commented at 1:01 PM on October 4, 2023: member

re-ACK 4cafe9f176e93ebb6c38abb12140e8d8be005cbf 🕗

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 4cafe9f176e93ebb6c38abb12140e8d8be005cbf  🕗
aVeTw3nWwOu1NhfkZrlDRhuBlLqkrBTg3gkDqrokTaAbgITq9SnnIz0C3g77xyoeq79UHglyBUf9fhLo8bMvDg==

</details>

glozow commented at 3:48 PM on October 4, 2023: member

concept && light code review ACK 4cafe9f176e93ebb6c38abb12140e8d8be005cbf

fanquake merged this on Oct 5, 2023

fanquake closed this on Oct 5, 2023

Frank-GER referenced this in commit e241eb4073 on Oct 13, 2023

Fabcien referenced this in commit e308d2e67b on Jul 16, 2024

bitcoin locked this on Oct 4, 2024