Skip to content

Conversation

amitiuttarwar
Copy link
Contributor

Introduce a Poisson helper method that wraps the existing method to return std::chrono::duration type, which is mockable.

Needed for #16698.

@fanquake fanquake added the P2P label Oct 24, 2019
@fanquake fanquake changed the title tools: add PoissonNextSend method that returns mockable time p2p: add PoissonNextSend method that returns mockable time Oct 28, 2019
@fanquake fanquake requested a review from maflcko October 28, 2019 14:28
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

Please switch nNextInvSend to the new type and use the new function there. Also, a unit test to assert that the two methods return the same value would be nice.

@amitiuttarwar
Copy link
Contributor Author

@MarcoFalke updated nNextInvSend. Not sure how to unit test since the result of the poisson method would be different for two separate calls. LMK if you want deeper changes with chrono types, happy to update.

@maflcko
Copy link
Member

maflcko commented Oct 31, 2019

You can set g_mock_deterministic_tests = true; for just your test case, then set it to false again at the end.

@amitiuttarwar
Copy link
Contributor Author

@MarcoFalke added tests. Ready for review.

@maflcko
Copy link
Member

maflcko commented Nov 1, 2019

ACK d504d70

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK d504d70e602bf30aad18e1e5677daa3961757bb7
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgYJAv+Nkm4e1Ac/wfPzNKwyAfkC7oRMCGqY1IqPm0vCugazxm+02u7JFotM4rI
j75ONrqy/LC7TAk7ELIWUfJv0lquBLGpkz5ZuYAcfAH8CjSj9NEt1bfm+XRmmXFe
jxLvL068KeDbz/6i2vwwQW3557/0A5p1z8zmTh66IXF7lQU/yjpSAOK5BXXxoVs8
sMqj2o1vZ7TMPZlGTxZxMQB49DUxdRi6TYMKssS7lJqrVbtTxHZcTwzdfOWWVg6e
epAow+pvO1GRV0yYjMzhAXH8HM3500GI3b66gVvvDpIbQK9+pvYDFuVadSHDYvpK
cbpbu5uXhaskeI5OOiv03L3v7lHrRQi/y01w03mD3SgGW66nE2YkN8L6+I5mk6Li
LovIQIvTr6x62xjt5YqYGZW+35RRYdIg1tueNKqmCSAYRGo7kBjUnDyCA3Y8/JGB
jv+afA/RUT2U+8lHYyZkV6rSr9HRuq16DKutyYGiz4VWeyGC0OtUCInf0fvnJkY8
380rfFsr
=RKgu
-----END PGP SIGNATURE-----

Timestamp of file with hash e887cddc7cfb3c4288a10e649645fa1687a1de16f54a8b1819200b2eaa1c86a8 -

Copy link

@ariard ariard left a comment

Choose a reason for hiding this comment

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

Can you explain in commit message why you switch for mockable time for outbound peers announcement ? I mean how it's going to be used in #16698

Copy link
Contributor

@ajtowns ajtowns left a comment

Choose a reason for hiding this comment

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

ACK d504d70 nits only

@ariard
Copy link

ariard commented Nov 4, 2019

ACK d504d70, checked directly on parent PR the rational of intended changes

@maflcko
Copy link
Member

maflcko commented Nov 4, 2019

@amitiuttarwar This has three ACKs, so is ready to go. Do you want to address the nits or leave them for later?

@naumenkogs
Copy link
Member

ACK d504d70
willing to do quick re-ACK if those tiny nits are resolved :)

@ajtowns
Copy link
Contributor

ajtowns commented Nov 5, 2019

ACK 1a8f0d5

@maflcko
Copy link
Member

maflcko commented Nov 5, 2019

re-ACK 1a8f0d5

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

re-ACK 1a8f0d5a74d5cc0000456932babf35301f5c1686
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjapAv+MgqGwdYAgJg/hzj7rhv2Fp+bXSWfAoF4Sgqeo7yZbQLfupqRev/uf3hP
DgC6rpyqIM2X8JuijUVxyOs2Z+bU8pOZMOpzVOGpZPHcgmBP1lVT+BqoV3iVgwD2
6dihYwbFOZIZz4jRyAn8Ca9Aoo4jRXY82kPKhhLfo4lRIcp19v0KzccyLeE3rwzh
Do7I5M0PISDid6t2xv57+8R+W5IZy5VChShEx3aHkGLVWR5Qwa01WnoPnPV5uQJO
amkT+i0i5TIRyyN8vSIJWLHfa7BJmG3cRAwekoq0hEeaFw3513hMFPtq+ekfOKb+
DdNFVFYPk5tkTfnYDHcjzxob2phFtEDpPxII8miRu1pFFhkSmcEUswEQZMCykcqi
CBddH8/oGs0G7yPjPtJ6GrmITsJHRwxhlxTyXarx3dm7F78AomLvDYRWUncSgGrh
X7k7PP2ZYTIZIe6YBiPQ1Y6zf73qdw52bsMtXhD5Fj9v/KTZMcW93IghKppmx6Hl
vnB9QRAE
=kQL2
-----END PGP SIGNATURE-----

Timestamp of file with hash c18dd002e27bda4c5087dbc652b61c6e0ece0bbd73a3416548d7cf0624aca3fb -

@naumenkogs
Copy link
Member

Is it a good idea to have both nNow and current_time? This might lead to counter-intuitive consequences, specifically in the test cases (I don't have the exact example right now).

@amitiuttarwar
Copy link
Contributor Author

amitiuttarwar commented Nov 5, 2019

@naumenkogs I'd like to work on replacing nNow in SendMessages entirely with current_time, but haven't prioritized yet bc I'm focused on rebroadcast stuff.

Are your concerns limited to the test case introduced or do you also have concerns about the code path? I don't see how problems could occur, so I'd love if you could explain further?


ah from IRL convo with AJ, he pointed out how it can be confusing from tests based on what functionality executes based on clock time vs needing to update mocktime. Is that what you're alluding to?

@naumenkogs
Copy link
Member

tests based on what functionality executes based on clock time vs needing to update mocktime. Is that what you're alluding to?

Yes. Also, potentially future code in net_processing.
At the same time, I guess it blocks you from proceeding with rebroadcast (even in terms of better testing) today.

ACK 1a8f0d5, and let's merge it and come back to it later.

maflcko pushed a commit that referenced this pull request Nov 5, 2019
1a8f0d5 [tools] update nNextInvSend to use mockable time (Amiti Uttarwar)
4de6303 [tools] add PoissonNextSend method that returns mockable time (Amiti Uttarwar)

Pull request description:

  Introduce a Poisson helper method that wraps the existing method to return `std::chrono::duration` type, which is mockable.

  Needed for #16698.

ACKs for top commit:
  ajtowns:
    ACK 1a8f0d5
  MarcoFalke:
    re-ACK 1a8f0d5
  naumenkogs:
    ACK 1a8f0d5, and let's merge it and come back to it later.

Tree-SHA512: 7e2325d7c55fc0b4357cb86b83e0c218ba269f678c1786342d8bc380bfd9696373bc24ff124b9ff17a6e761c62b2b44ff5247c3911e2afdc7cc5c20417e8290b
@maflcko maflcko merged commit 1a8f0d5 into bitcoin:master Nov 5, 2019
@amitiuttarwar amitiuttarwar deleted the 1910-mockable-poisson branch November 5, 2019 18:53
codablock pushed a commit to codablock/dash that referenced this pull request Apr 9, 2020
…kable time

1a8f0d5 [tools] update nNextInvSend to use mockable time (Amiti Uttarwar)
4de6303 [tools] add PoissonNextSend method that returns mockable time (Amiti Uttarwar)

Pull request description:

  Introduce a Poisson helper method that wraps the existing method to return `std::chrono::duration` type, which is mockable.

  Needed for bitcoin#16698.

ACKs for top commit:
  ajtowns:
    ACK 1a8f0d5
  MarcoFalke:
    re-ACK 1a8f0d5
  naumenkogs:
    ACK 1a8f0d5, and let's merge it and come back to it later.

Tree-SHA512: 7e2325d7c55fc0b4357cb86b83e0c218ba269f678c1786342d8bc380bfd9696373bc24ff124b9ff17a6e761c62b2b44ff5247c3911e2afdc7cc5c20417e8290b
codablock pushed a commit to codablock/dash that referenced this pull request Apr 12, 2020
…kable time

1a8f0d5 [tools] update nNextInvSend to use mockable time (Amiti Uttarwar)
4de6303 [tools] add PoissonNextSend method that returns mockable time (Amiti Uttarwar)

Pull request description:

  Introduce a Poisson helper method that wraps the existing method to return `std::chrono::duration` type, which is mockable.

  Needed for bitcoin#16698.

ACKs for top commit:
  ajtowns:
    ACK 1a8f0d5
  MarcoFalke:
    re-ACK 1a8f0d5
  naumenkogs:
    ACK 1a8f0d5, and let's merge it and come back to it later.

Tree-SHA512: 7e2325d7c55fc0b4357cb86b83e0c218ba269f678c1786342d8bc380bfd9696373bc24ff124b9ff17a6e761c62b2b44ff5247c3911e2afdc7cc5c20417e8290b
codablock pushed a commit to codablock/dash that referenced this pull request Apr 12, 2020
…kable time

1a8f0d5 [tools] update nNextInvSend to use mockable time (Amiti Uttarwar)
4de6303 [tools] add PoissonNextSend method that returns mockable time (Amiti Uttarwar)

Pull request description:

  Introduce a Poisson helper method that wraps the existing method to return `std::chrono::duration` type, which is mockable.

  Needed for bitcoin#16698.

ACKs for top commit:
  ajtowns:
    ACK 1a8f0d5
  MarcoFalke:
    re-ACK 1a8f0d5
  naumenkogs:
    ACK 1a8f0d5, and let's merge it and come back to it later.

Tree-SHA512: 7e2325d7c55fc0b4357cb86b83e0c218ba269f678c1786342d8bc380bfd9696373bc24ff124b9ff17a6e761c62b2b44ff5247c3911e2afdc7cc5c20417e8290b
codablock pushed a commit to codablock/dash that referenced this pull request Apr 12, 2020
…kable time

1a8f0d5 [tools] update nNextInvSend to use mockable time (Amiti Uttarwar)
4de6303 [tools] add PoissonNextSend method that returns mockable time (Amiti Uttarwar)

Pull request description:

  Introduce a Poisson helper method that wraps the existing method to return `std::chrono::duration` type, which is mockable.

  Needed for bitcoin#16698.

ACKs for top commit:
  ajtowns:
    ACK 1a8f0d5
  MarcoFalke:
    re-ACK 1a8f0d5
  naumenkogs:
    ACK 1a8f0d5, and let's merge it and come back to it later.

Tree-SHA512: 7e2325d7c55fc0b4357cb86b83e0c218ba269f678c1786342d8bc380bfd9696373bc24ff124b9ff17a6e761c62b2b44ff5247c3911e2afdc7cc5c20417e8290b
codablock pushed a commit to codablock/dash that referenced this pull request Apr 14, 2020
…kable time

1a8f0d5 [tools] update nNextInvSend to use mockable time (Amiti Uttarwar)
4de6303 [tools] add PoissonNextSend method that returns mockable time (Amiti Uttarwar)

Pull request description:

  Introduce a Poisson helper method that wraps the existing method to return `std::chrono::duration` type, which is mockable.

  Needed for bitcoin#16698.

ACKs for top commit:
  ajtowns:
    ACK 1a8f0d5
  MarcoFalke:
    re-ACK 1a8f0d5
  naumenkogs:
    ACK 1a8f0d5, and let's merge it and come back to it later.

Tree-SHA512: 7e2325d7c55fc0b4357cb86b83e0c218ba269f678c1786342d8bc380bfd9696373bc24ff124b9ff17a6e761c62b2b44ff5247c3911e2afdc7cc5c20417e8290b
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Nov 4, 2020
Summary:
> Introduce a Poisson helper method that wraps the existing method to return std::chrono::duration type, which is mockable.

This is a backport of Core [[bitcoin/bitcoin#17243 | PR17243]]

Test Plan: `ninja && ninja check`

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, majcosta

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, majcosta

Subscribers: majcosta

Differential Revision: https://reviews.bitcoinabc.org/D8252
random-zebra added a commit to PIVX-Project/PIVX that referenced this pull request Jul 15, 2021
…sync fixes

f57de3f net_processing: Use constant MAX_ADDR_TO_SEND instead of harcoded value. (furszy)
aee606a Add missing peer whitelist flag set for manual connections. (furszy)
a2c8716 Test: Add peers whitelist for tests using mocked time. (furszy)
cc3b7bb Refactor: Move NetEventsInterface class below CNode declaration so it can get access to the cs_sendProcessing field and detect missing cs_sendProcessing locks properly. (furszy)
76f4a0c Test: Add missing SendMessages required cs locks (furszy)
00a685c [tools] update nNextInvSend to use mockable time (Amiti Uttarwar)
3b832c9 [tools] add PoissonNextSend method that returns mockable time (Amiti Uttarwar)

Pull request description:

  Another decouple from Tor's v3 addresses support (#2411).

  First two commits are introducing and connecting a helper poisson method so the trickle logic works with mocked time as well (from bitcoin#17243).
  The third and fourth commit solves missing `cs_processing` locks in the DoS tests and compiler checks.
  The fifth commit solves mempool sync issues in the functional tests that utilize a mocked time.

ACKs for top commit:
  random-zebra:
    utACK f57de3f

Tree-SHA512: 9b33f22aab6f4b1e8e72a1520432ede6d0672b4ec060f7f53dfa7f82d138c606b58c7f2ca794b355dafcfaee6b65eb21a60183196b563a1c8523adebafcaf059
random-zebra added a commit to PIVX-Project/PIVX that referenced this pull request Aug 11, 2021
ecde04a [Consensus] Bump Active Protocol version to 70923 for v5.3 (random-zebra)
b63e4f5 Consensus: Add v5.3 enforcement height for testnet. (furszy)
f44be94 Only relay IPv4, IPv6, Tor addresses (Pieter Wuille)
015298c fix: tor: Call event_base_loopbreak from the event's callback (furszy)
34ff7a8 Consensus: Add mnb ADDRv2 guard. (furszy)
b4515dc GUI: Present v3 onion addresses properly in MNs list. (furszy)
337d43d tests: don't export in6addr_loopback (Vasil Dimov)
2cde8e0 GUI: Do not show the tor v3 onion address in the topbar. (furszy)
0b5f406 Doc: update tor.md with latest upstream information. (furszy)
89df7f2 addrman: ensure old versions don't parse peers.dat (Vasil Dimov)
bb90c5c test: add getnetworkinfo network name regression tests (Jon Atack)
d8e01b5 rpc: update GetNetworksInfo() to not return unsupported networks (Jon Atack)
57fc7b0 net: update GetNetworkName() with all enum Network cases (Jon Atack)
647d60b tests: Modify rpc_bind to conform to bitcoin#14532 behaviour. (Carl Dong)
d4d6729 Allow running rpc_bind.py --nonloopback test without IPv6 (Kristaps Kaupe)
4a034d8 test: Add rpc_bind test to default-run tests (Wladimir J. van der Laan)
61a08af [tests] bind functional test nodes to 127.0.0.1  Prevents OSX firewall (Sjors Provoost)
6a4f1e0 test: Add basic addr relay test (furszy)
78aa61c net: Make addr relay mockable (furszy)
ba954ca Send and require SENDADDRV2 before VERACK (Pieter Wuille)
61c2ed4 Bump net protocol version + don't send 'sendaddrv2' to pre-70923 software (furszy)
ccd508a tor: make a TORv3 hidden service instead of TORv2 (Vasil Dimov)
6da9a14 net: advertise support for ADDRv2 via new message (furszy)
e58d5d0 Migrate to test_large_inv() to Misbehaving logging. (furszy)
d496b64 [QA] fix mininode CAddress ser/deser (Jonas Schnelli)
cec9567 net: CAddress & CAddrMan: (un)serialize as ADDRv2 Change the serialization of `CAddrMan` to serialize its addresses in ADDRv2/BIP155 format by default. Introduce a new `CAddrMan` format version (3). (furszy)
b8c1dda streams update: get rid of nType and nVersion. (furszy)
3eaa273 Support bypassing range check in ReadCompactSize (Pieter Wuille)
a237ba4 net: recognize TORv3/I2P/CJDNS networks (Vasil Dimov)
8e50853 util: make EncodeBase32 consume Spans (Sebastian Falbesoner)
1f67e30 net: CNetAddr: add support to (un)serialize as ADDRv2 (Vasil Dimov)
2455420 test: move HasReason so it can be reused (furszy)
d41adb4 util: move HasPrefix() so it can be reused (Vasil Dimov)
f6f86af Unroll Keccak-f implementation (Pieter Wuille)
45222e6 Implement keccak-f[1600] and SHA3-256 (Pieter Wuille)
08ad06d net: change CNetAddr::ip to have flexible size (furszy)
3337219 net: improve encapsulation of CNetAddr. (furszy)
910d5c4 test: Do not instantiate CAddrDB for static call (Hennadii Stepanov)
6b607ef Drop IsLimited in favor of IsReachable (Ben Woosley)
a40711b IsReachable is the inverse of IsLimited (DRY). Includes unit tests (marcaiaf)
8839828 net: don't accept non-left-contiguous netmasks (Vasil Dimov)
5d7f864 rpcbind: Warn about exposing RPC to untrusted networks (Luke Dashjr)
2a6abd8 CNetAddr: Add IsBindAny method to check for INADDR_ANY (Luke Dashjr)
4fdfa45 net: Always default rpcbind to localhost, never "all interfaces" (Luke Dashjr)
31064a8 net: Minor accumulated cleanups (furszy)
9f9c871 tests: Avoid using C-style NUL-terminated strings as arguments (practicalswift)
f6c52a3 tests: Add tests to make sure lookup methods fail on std::string parameters with embedded NUL characters (practicalswift)
a751b9b net: Avoid using C-style NUL-terminated strings as arguments in the netbase interface (furszy)
f30869d test: add IsRFC2544 tests (Mark Tyneway)
ed5abe1 Net: Proper CService deserialization + GetIn6Addr return false if addr isn't an IPv6 addr (furszy)
86d73fb net: save the network type explicitly in CNetAddr (Vasil Dimov)
ad57dfc net: document `enum Network` (Vasil Dimov)
cb160de netaddress: Update CNetAddr for ORCHIDv2 (Carl Dong)
c3c04e4 net: Better misbehaving logging (furszy)
3660487 net: Use C++11 member initialization in protocol (Marco)
082baa3 refactor: Drop unused CBufferedFile::Seek() (Hennadii Stepanov)
e2d776a util: CBufferedFile fixes (Larry Ruane)
6921f42 streams: backport OverrideStream class (furszy)

Pull request description:

  Conjunction of a large number of back ports, updates and refactorings that made with the final goal of implementing v3 Onion addresses support (BIP155 https://github.com/bitcoin/bips/blob/master/bip-0155.mediawiki) before the tor v2 addresses EOL, scheduled, by the Tor project, for (1) July 15th: v2 addr support removal from the code base, and (2) October 15th: v2 addr network disable, where **every peer in our network running under Tor will loose the connection and drop the network**.

  As BIP155 describes, this is introducing a new P2P message to gossip longer node addresses over the P2P network. This is required to support new-generation Onion addresses, I2P, and potentially other networks that have longer endpoint addresses than fit in the 128 bits of the current addr message.

  In order to achieve the end goal, had to:
  1.  Create Span class and push it up to latest Bitcoin implementation.
  2.  Update the whole serialization framework and every object using it up to latest Bitcoin implementation (3-4 years ahead of what we currently are in master).
  3.  Update the address manager implementing ASN-based bucketing of the network nodes.
  4.  Update and refactor the netAddress and address manager tests to latest Bitcoin implementation (4 years ahead of what we currently are in master).
  5.  Several util string, vector, encodings, parsing, hashing backports and more..

  Important note:
  This PR it is not meant to be merged as a standalone PR, will decouple smaller ones moving on. Adding on each sub-PR its own description isolated from this big monster.

  Second note:
  This is still a **work-in-progress**, not ready for testing yet. I'm probably missing to mention few PRs that have already adapted to our sources. Just making it public so can decouple the changes, we can start merging them and i can continue working a bit more confortable (rebase a +170 commits separate branch is not fun..).

  ### List of back ported and adapted PRs:

  Span and Serialization:
  ----------------
  *  bitcoin#12886.
  *  bitcoin#12916.
  *  bitcoin#13558.
  *  bitcoin#13697. (Only Span's commit 29943a9)
  *  bitcoin#17850.
  *  bitcoin#17896.
  *  bitcoin#12752.
  *  bitcoin#16577.
  *  bitcoin#16670. (without faebf62)
  *  bitcoin#17957.
  *  bitcoin#18021.
  *  bitcoin#18087.
  *  bitcoin#18112 (only from 353f376 that we don't support).
  *  bitcoin#18167.
  *  bitcoin#18317.
  *  bitcoin#18591 (only Span's commit 0fbde48)
  *  bitcoin#18468.
  *  bitcoin#19020.
  *  bitcoin#19032.
  *  bitcoin#19367.
  *  bitcoin#19387.

  Net, NetAddress and AddrMan:
  ----------------

  *  bitcoin#7932.
  *  bitcoin#10756.
  *  bitcoin#10765.
  *  bitcoin#12218.
  *  bitcoin#12855.
  *  bitcoin#13532.
  *  bitcoin#13575.
  *  bitcoin#13815.
  *  bitcoin#14532.
  *  bitcoin#15051.
  *  bitcoin#15138.
  *  bitcoin#15689.
  *  bitcoin#16702.
  *  bitcoin#17243.
  *  bitcoin#17345.
  *  bitcoin#17754.
  *  bitcoin#17758.
  *  bitcoin#17812.
  *  bitcoin#18023.
  *  bitcoin#18454.
  *  bitcoin#18512.
  *  bitcoin#19314.
  *  bitcoin#19687

  Keys and Addresses encoding:
  ----------------
  * bitcoin#11372.
  * bitcoin#17511.
  * bitcoin#17721.

  Util:
  ----------------
  * bitcoin#9140.
  * bitcoin#16577.
  * bitcoin#16889.
  * bitcoin#19593.

  Bench:
  ----------------
  * bitcoin#16299.

  BIP155:
  ----------------
  *  bitcoin#19351.
  *  bitcoin#19360.
  *  bitcoin#19534.
  *  bitcoin#19628.
  *  bitcoin#19841.
  *  bitcoin#19845.
  *  bitcoin#19954.
  *  bitcoin#19991 (pending).
  *  bitcoin#19845.
  *  bitcoin#20000 (pending).
  *  bitcoin#20120.
  *  bitcoin#20284.
  *  bitcoin#20564.
  *  bitcoin#21157 (pending).
  *  bitcoin#21564 (pending).
  *  Fully removed v2 onion addr support.
  *  Add hardcoded seeds.
  *  Add release-notes, changes to files.md and every needed documentation.

  I'm currently working on the PRs marked as "pending", this isn't over, but I'm pretty pretty close :). What a long road..

ACKs for top commit:
  random-zebra:
    utACK ecde04a
  Fuzzbawls:
    ACK ecde04a

Tree-SHA512: 82c95fbda76fce63f96d8a9af7fa9a89cb1e1b302b7891e27118a6103af0be23606bf202c7332fa61908205e6b6351764e2ec23d753f1e2484028f57c2e8b51a
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants