BIP324: Enable v2 P2P encrypted transport #24545

pull dhruv wants to merge 37 commits into bitcoin:master from dhruv:bip324-enable changing 78 files +5075 −791
  1. dhruv commented at 6:43 pm on March 12, 2022: contributor

    This PR brings together all other BIP324 PRs and enables v2 encrypted P2P transport.

    Builds on top of PRs: #25361, #23233, #23561, #23432. It looks like there’s a lot of commits, but only the last 12 commits belong in this PR. The rest will be merged with upstream PRs.

    The dependency tree for BIP324 PRs is here.

    BIP324 is here.

    Running a v2 node

    Get the code

    0git remote add bip324 git@github.com:dhruv/bitcoin.git
    1git fetch bip324
    2git checkout bip324/bip324-enable
    

    Build for your OS

    Follow the appropriate instructions here

    Run the node

    0src/bitcoind -conf=CONFIG_FILE -v2transport=1
    

    Connect with a friend’s v2 node

    0src/bitcoin-cli -conf=CONFIG_FILE addnode "FRIEND_IP:FRIEND_PORT" "add" true
    

    The last parameter(p2p_v2:true) signals to your node that the peer is running a v2 supportive client and we should attempt to make an encrypted P2P connection (you’re simulating the NODE_P2P_V2 service flag advertisement manually). Should that fail however (say because the peer told you mistakenly, lied, etc.), this code will downgrade the connection to unencrypted v1 transport.

    Things you are helpful to test

    • If your friend’s node is a v2 node, you can see with wireshark that the bytes are pseudorandom (the easiest way to confirm this is that with a v1 connection, wireshark will tell you it has detected a Bitcoin connection and it’ll even parse out the metadata like message type, etc; with v2, wireshark has no idea – of course that could be because wireshark does simply not know v2, but it is because the bytestream is pseudorandom)
    • Compare the v2 encrypted session id exposed via getpeerinfo as v2_session_id with your friend.
    • Add another peer that is actually v1, but try addnode still indicating v2 support. You should see with wireshark that after a failed attempt at a v2 handshake, the connection is downgraded to unencrypted v1 and wireshark can parse it.

    I’ve been told there are v2 nodes running at (happy to update the list as more people run persistent v2 nodes; message me and I’ll add it here):

     0be.anyone.eu.org
     1rp7k2go3s5lyj3fnj6zn62ktarlrsft2ohlsxkyd7v3e3idqyptvread.onion:8333
     2jdcoysubtxazi7dketpyb5rnjorvxad4onftveohash2pdwkgw4bvnqd.onion:8333
     3xci6cphki2pdb5qe7axzrcxcxabkbm24z4zlv2hn4ziy6grquqco2kyd.onion:8333
     4slvtesfgg3mkksqqzh67al4sq6dx3rhlzqepa4ny7jonzuckg6msf3id.onion:8333
     5gifm4fnj3vua664xhgeanx5fnpco3txkqy4amr4txbfsciiyrkxpf2qd.onion:8333
     6300:5ecb:6b8a:d837::3:8333
     7300:5ecb:6b8a:d837::a6d6:8333
     82001:470:1f1a:365::2:8333
     92001:470:1f1b:365:aa20:66ff:fe3f:1909:8333
    10184.74.240.157:8533
    1195.179.145.232:8333
    
  2. fanquake added the label P2P on Mar 12, 2022
  3. DrahtBot commented at 9:00 pm on March 12, 2022: 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
    Concept ACK jonatack, dergoegge

    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:

    • #27294 (refactor: Move chain names to the kernel namespace by TheCharlatan)
    • #27257 (refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg by dergoegge)
    • #27254 (refactor: Extract util/fs from util/system by TheCharlatan)
    • #27114 (p2p: Allow whitelisting outgoing connections by brunoerg)
    • #26728 (wallet: Have the wallet store the key for automatically generated descriptors by achow101)
    • #26684 (bench: add readblock benchmark by andrewtoth)
    • #26621 (refactor: Continue moving application data from CNode to Peer by dergoegge)
    • #26593 (tracing: Only prepare tracepoint arguments when actually tracing by 0xB10C)
    • #26366 (p2p, rpc, test: getpeerinfo improv + add test coverage for invalid command by brunoerg)
    • #26283 (p2p: Fill reconciliation sets and request reconciliation (Erlay) by naumenkogs)
    • #26261 (p2p: cleanup LookupIntern, Lookup and LookupHost by brunoerg)
    • #25907 (wallet: rpc to add automatically generated descriptors by achow101)
    • #25832 (tracing: network connection tracepoints by 0xB10C)
    • #25572 (refactor: Introduce EvictionManager and use it for the inbound eviction logic by dergoegge)
    • #25390 (sync: introduce a thread-safe generic container and use it to remove a bunch of “GlobalMutex"es by vasild)
    • #25284 (net: Use serialization parameters for CAddress serialization by MarcoFalke)
    • #25268 (refactor: Introduce EvictionManager by dergoegge)
    • #23352 (test: Extend stale_tip_peer_management test by MarcoFalke)
    • #22341 (rpc: add getxpub by Sjors)
    • #18470 (test: Extend stale tip test by MarcoFalke)
    • #10102 (Multiprocess bitcoin by ryanofsky)

    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.

  4. dhruv force-pushed on Mar 21, 2022
  5. dhruv commented at 8:14 pm on March 21, 2022: contributor
    Bringing in upstream changes from #20962. Ready for further review.
  6. DrahtBot added the label Needs rebase on Mar 25, 2022
  7. dhruv force-pushed on Mar 30, 2022
  8. dhruv commented at 4:13 am on March 30, 2022: contributor

    Rebased to resolve conflicts with #21160.

    Addressed comments from @mzumsande and @kallewoof in #23900.

    Ready for further review.

    git range-diff 171f6f269 4a38f3e be9a830

  9. DrahtBot removed the label Needs rebase on Mar 30, 2022
  10. DrahtBot added the label Needs rebase on Mar 30, 2022
  11. dhruv force-pushed on Mar 30, 2022
  12. dhruv commented at 4:44 pm on March 30, 2022: contributor

    Rebased. Ready for further review.

    git range-diff 171f6f269 be9a830 f7cae59

  13. DrahtBot removed the label Needs rebase on Mar 30, 2022
  14. jonatack commented at 12:08 pm on April 4, 2022: member
    Concept ACK
  15. laanwj commented at 5:50 pm on April 7, 2022: member

    I’m testing this on one of my nodes. Feel free to message me if you want to connect.

    Is there any way to see (in, say, getpeerinfo) whether a connection uses P2P v2?

  16. dhruv commented at 6:24 pm on April 7, 2022: contributor

    Thanks, @laanwj !

    Is there any way to see (in, say, getpeerinfo) whether a connection uses P2P v2?

    That’s a good idea, I’ve added it here: https://github.com/bitcoin/bitcoin/projects/17#card-80335229 I’ll work on it once I’m done with the basics. For now, can you tcpdump -Apn -i any -w - -U port <PORT> | wireshark -k -i - to see the differences between v1 and v2?

  17. DrahtBot added the label Needs rebase on Apr 8, 2022
  18. dhruv force-pushed on Apr 9, 2022
  19. dhruv commented at 3:50 pm on April 9, 2022: contributor

    Rebased. git range-diff e0680bbce f7cae59 d0aef7a1d

    Ready for further review.

  20. DrahtBot removed the label Needs rebase on Apr 9, 2022
  21. DrahtBot added the label Needs rebase on Apr 9, 2022
  22. bitcoin deleted a comment on Apr 12, 2022
  23. 0xB10C commented at 9:07 am on April 28, 2022: contributor
    I’m running d0aef7a1d2f324aad923f734247d34baf9a50ae8 on the node behind https://bitcoind.observer. DM me for the IP.
  24. 0xB10C commented at 1:21 pm on April 28, 2022: contributor

    From what I can tell, my node fails to connect to another p2p v2 peer (likely @laanwj’s, we haven’t fully confirmed that yet) and stops accepting inbound messages from any other peer cutting it off from the network entirely.

    Logs:

     0(node operating as ususal)
     1..
     22022-04-28T12:25:39Z got inv: wtx c0b2943d5e9bf180088a6683011c04bbee1e9485fdc35eae408cd30614e392a2  have peer=6
     32022-04-28T12:25:39Z got inv: wtx 842fb44f4669248d8299523f6f2660c4717a11293b986973cc227ace30af8406  have peer=6
     42022-04-28T12:25:39Z sending inv (289 bytes) peer=6
     52022-04-28T12:25:39Z Making feeler connection to XXX.XXX.XXX.XXX:8333
     62022-04-28T12:25:39Z trying connection XXX.XXX.XXX.XXX:8333 lastseen=188.3hrs
     72022-04-28T12:25:39Z Added connection to XXX.XXX.XXX.XXX:8333 peer=31
     82022-04-28T12:25:39Z socket closed for peer=31
     92022-04-28T12:25:39Z disconnecting peer=31                      <-------- breaks here
    102022-04-28T12:25:40Z sending inv (325 bytes) peer=4
    112022-04-28T12:25:41Z sending inv (361 bytes) peer=27
    122022-04-28T12:25:41Z sending inv (793 bytes) peer=24
    132022-04-28T12:25:41Z Requesting tx 1bd8401dac5fb4b922a95c6c939cbf901d946ab3a6d2c811fd498f8c743bdd23 peer=3
    142022-04-28T12:25:41Z Requesting tx ca432d826b5163bf78320eb9cecf2f47aab8cb8f176fa67e4991e659e69e7c6e peer=3
    152022-04-28T12:25:41Z sending getdata (73 bytes) peer=3
    162022-04-28T12:25:43Z sending inv (109 bytes) peer=3
    172022-04-28T12:25:44Z sending inv (397 bytes) peer=11
    182022-04-28T12:25:44Z sending inv (721 bytes) peer=18
    192022-04-28T12:25:46Z sending inv (397 bytes) peer=26
    202022-04-28T12:25:47Z sending inv (397 bytes) peer=13
    212022-04-28T12:25:47Z sending inv (541 bytes) peer=5
    22..
    23(no more inbound messages received)
    
  25. dhruv commented at 9:18 pm on April 28, 2022: contributor
    @0xB10C Thanks for the logs. I’ll look into it. We are making some large changes to the spec at the moment, so I’m working on updating the BIP and some of the branches are actually out of sync now. I’ll be back in touch soon with updated code.
  26. theStack commented at 1:16 pm on May 30, 2022: contributor
    @dhruv: I’d have interest in testing and reviewing BIP324. Is there anything useful we can do for now or is it better to just wait for the final spec / updated BIP? (Maybe some PRs are not affected by the changes and can be already reviewed?)
  27. dhruv commented at 6:41 pm on May 30, 2022: contributor
    @theStack awesome! I’m working on changes you mentioned. In the meantime #23432 and upstream elligator squared changes in secp are unaffected and you can start review there. #23561 will have very minimal changes so you can review now and the updates will be easy to review. #20962 and #23233 will be radically different.
  28. laanwj commented at 6:53 am on June 1, 2022: member

    or is it better to just wait for the final spec / updated BIP

    Waiting is not a good option :smile: I think it’s unavoidable that the both the BIP and implementation will still evolve a bit during review and testing, as new issues are discovered. In FOSS there’s pretty much never a straightforward spec → implementation flow.

  29. theStack commented at 1:19 pm on June 1, 2022: contributor

    or is it better to just wait for the final spec / updated BIP

    Waiting is not a good option 😄 I think it’s unavoidable that the both the BIP and implementation will still evolve a bit during review and testing, as new issues are discovered. In FOSS there’s pretty much never a straightforward spec → implementation flow.

    Agree! My concern was that the undergoing spec changes are possibly so intense that the current publicly available BIP barely has any resemblance with the final updated version anymore and reviewing/testing current PRs would not be a well-invested time. But fortunately, that doesn’t seem to be the case (https://github.com/bitcoin/bitcoin/pull/24545#issuecomment-1141408247, https://bip324.com/sections/code-review/), i.e. no excuses for waiting :)

  30. ExperiBass commented at 5:07 pm on July 22, 2022: none

    From what I can tell, my node fails to connect to another p2p v2 peer (likely @laanwj’s, we haven’t fully confirmed that yet) and stops accepting inbound messages from any other peer cutting it off from the network entirely.

    I also seem to have this issue. Relevant logs:

     02022-07-22T13:55:24Z received: inv (109 bytes) peer=19
     12022-07-22T13:55:24Z got inv: tx cae283722e02ae9aa0e0da00b8064b76bf05583d17d8b8b4bc8dc84578a77a3b  new peer=19
     22022-07-22T13:55:24Z got inv: tx ee58f2752d52e7fc79e9e19ddc9b592c4fb105cced7f5c245175eb0615bf2139  new peer=19
     32022-07-22T13:55:24Z got inv: tx 3f1a7195723b7a711aac44c43ff12a6a3e360bacf52f685d0c5f1e706c64de3f  new peer=19
     42022-07-22T13:55:24Z socket closed for peer=16
     52022-07-22T13:55:24Z disconnecting peer=16
     62022-07-22T13:55:24Z Cleared nodestate for peer=16
     72022-07-22T13:55:25Z received: version (102 bytes) peer=23
     82022-07-22T13:55:25Z sending wtxidrelay (0 bytes) peer=23
     92022-07-22T13:55:25Z sending sendaddrv2 (0 bytes) peer=23
    102022-07-22T13:55:25Z sending verack (0 bytes) peer=23
    11(no more inbound)
    

    Full log

  31. in src/net.cpp:1929 in d0aef7a1d2 outdated
    1929+                        // Send empty message for transport version placeholder
    1930+                        CSerializedNetMsg msg;
    1931+                        PushMessage(pnode, std::move(msg));
    1932+
    1933+                        if (!pnode->IsInboundConn()) {
    1934+                            m_msgproc->InitP2P(*pnode, !pnode->IsBlockOnlyConn());
    


    dergoegge commented at 10:29 am on July 25, 2022:
    0                            m_msgproc->InitializeNode(pnode);
    
  32. in src/net.cpp:2613 in d0aef7a1d2 outdated
    2605@@ -2285,6 +2606,10 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai
    2606     if (grantOutbound)
    2607         grantOutbound->MoveTo(pnode->grantOutbound);
    2608 
    2609+    // Only the outbound peer knows that both sides support BIP324 transport
    2610+    if (pnode->PreferV2Conn()) {
    2611+        PushV2EllSqPubkey(pnode);
    2612+    }
    2613     m_msgproc->InitializeNode(pnode);
    


    dergoegge commented at 10:31 am on July 25, 2022:
    0    } else {
    1        m_msgproc->InitializeNode(pnode);
    2    }
    
  33. in src/net_processing.cpp:1236 in d0aef7a1d2 outdated
    1232@@ -1235,8 +1233,8 @@ void PeerManagerImpl::InitializeNode(CNode *pnode)
    1233         LOCK(m_peer_mutex);
    1234         m_peer_map.emplace_hint(m_peer_map.end(), nodeid, peer);
    1235     }
    1236-    if (!pnode->IsInboundConn()) {
    1237-        PushNodeVersion(*pnode, *peer);
    1238+    if (!pnode->IsInboundConn() && !pnode->PreferV2Conn()) {
    


    dergoegge commented at 10:33 am on July 25, 2022:
    0    if (!pnode->IsInboundConn()) {
    
  34. dergoegge commented at 11:20 am on July 25, 2022: member
    I think you can drop the net_processing changes if you don’t call InitializeNode for outbound connections until it is clear which transport version will be used (see my review comments, they only make sense together). I think this is safe w.r.t only calling InitializeNode once but please double check.
  35. dhruv force-pushed on Sep 12, 2022
  36. dhruv commented at 4:13 am on September 12, 2022: contributor

    Changes in this push:

    • Bring in upstream changes
    • Added shapable handshake per changes developed in the working group.
    • Added a functional test

    Ready for further review.

    Next up, I’ll be looking into the review from @dergoegge and the logs from @0xB10C and @ExperiBass.

  37. DrahtBot removed the label Needs rebase on Sep 12, 2022
  38. dhruv force-pushed on Sep 19, 2022
  39. dhruv commented at 5:57 pm on September 19, 2022: contributor

    @ExperiBass, I have worked with @0xB10C and @stratospher to find and fix the thread locking issue. Are you able to try again with the latest code?

    This push:

    • Fixed thread locking issue that happened when the Initiator tried to downgrade a v2 BIP324 connection to a v1 plaintext connection whilst the outbound slots were full.
    • Fixed another bug where the Initiator would bot send the VERSION message on a v2 connection if there were no more inbound bytes to process. This wasn’t caught by the functional test because there were more inbound bytes to process.

    Next up, comments from @dergoegge. Ready for further review.

  40. DrahtBot added the label Needs rebase on Sep 20, 2022
  41. ExperiBass commented at 10:58 pm on September 20, 2022: none

    @ExperiBass, I have worked with @0xB10C and @stratospher to find and fix the thread locking issue. Are you able to try again with the latest code?

    So far so good, i havent seen any crashes for the past 24 hours. Looks like its fine 👍🏾

  42. dhruv force-pushed on Oct 5, 2022
  43. dhruv commented at 10:03 pm on October 5, 2022: contributor

    Latest push:

    • Rebased
    • Nomenclature update
    • Bring in upstream changes
    • Interpret encrypted length as length of contents rather than header + contents
    • Rekey salt length is now 23 bytes instead of 32 bytes
    • New test vectors, including a trivial one-byte vector and a large vector
    • Authentication of garbage sent by initiator in the shapable handshake
  44. DrahtBot removed the label Needs rebase on Oct 5, 2022
  45. DrahtBot added the label Needs rebase on Oct 17, 2022
  46. dhruv force-pushed on Oct 21, 2022
  47. dhruv commented at 6:03 am on October 21, 2022: contributor

    Latest push brings all the code up to date with the released BIP draft. Changes:

    Handshake shapability

    • Bidirectional garbage and terminators
    • Early key response by responder upon first mismatching byte
    • Allowing for multiple rounds in key exchange
    • Remove bias away from NETWORK_MAGIC || “version\x00” for ElligatorSwift encodings
    • Remove garbage terminator from the garbage authentication packet as it is implicitly authenticated

    Bugfixes

    • An already established v1 connection peer should not send out an ellswift key if a message has a bad v1 header
    • The previous changes to test_framewotk.py made all tests fail with a small probability, that’s now fixed

    Test vectors

    • One-sided test vectors consistent with the BIP (the BIP actually needs updating which is WIP)
    • Test vectors in a json file instead of making net_tests.cpp even longer

    RPC

    • Expose transport protocol type via getpeerinfo
    • Expose v2 session id via getpeerinfo

    Also rebased the PR.

    Ready for further review. Changes in this push are not backwards compatible with the previous code(handshake shapability changes) so it’d be nice if everyone running the branch could run the latest version.

  48. DrahtBot removed the label Needs rebase on Oct 21, 2022
  49. dhruv force-pushed on Oct 25, 2022
  50. dhruv commented at 4:19 pm on October 25, 2022: contributor

    Latest push fixes a bug that was causing non-deterministic integration test failures. Initially, I thought it was intermittent integration test failures but it was a bug in the code for BIP324 implementation. This comment explains for interested readers and documentation.

    v2 supportive inbound clients do not know when the first bytes come in whether the initiator intends on a v1 or a v2 connection. So if they have not received a VERSION message and the first message is not a VERSION message (assuming the v1 message header is valid, except for the checksum), the bytes must be interpreted as an ellswift pubkey (this is also the case if the previous bytes in the header are invalid, like say the network magic bytes are not correct). However, I was doing this by checking nVersion == 0 rather than mapRecvBytesPerMsgType[VERSION] == 0 which caused a race condition because a VERSION message could be received, and not yet processed (which happens asynchronously). The way this manifest was that a P2PConnection would send VERSION to a TestNode, TestNode would send VERSION back, and the P2PConnection would send a VERACK. When that VERACK was received at the TestNode, there was a chance that the VERSION message was queued, but not processed yet, so nVersion was still zero. The code would interpret this as a bad first message and disconnect causing add_p2p_connection() to fail with assert p2p_conn.is_connected while it was in wait_until(VERACK).

    The other problem this discovered was that there is no need to change the behavior of v1 clients where the code is v2 aware, but not signalling support.

    You can see the bug fix with git diff 601fb3a 4f23845

    Ready for further review.

  51. dhruv force-pushed on Oct 25, 2022
  52. dhruv commented at 5:55 pm on October 25, 2022: contributor

    Pushed again: tests against previous releases (which don’t run on my personal fork for some reason) were failing because of the new optional argument to rpc:addnode not existing in older clients.

    git diff 4f23845 e823a19

  53. dhruv force-pushed on Oct 27, 2022
  54. dhruv force-pushed on Oct 27, 2022
  55. dhruv commented at 10:03 pm on October 27, 2022: contributor

    Latest push adds a downgrade to v1 upon inactivity within the peer timeout period (the case where a peer is falsely advertised as v2 supportive, but also doesn’t disconnect when it received an ellswift pubkey). Also changed the order of commits.

    git diff e823a19d59 062e51c2f0

  56. jamesob commented at 5:21 pm on October 29, 2022: member
    Docker image (jamesob/bitcoind:bip324-enable) built for this branch as of 062e51c2f051dc68197bc21535fd06b3e01bdc21, in case that helps anyone test.
  57. rot13maxi commented at 0:21 am on October 31, 2022: none
    built the dhruv:bip324-enable at commit 062e51c2f051dc68197bc21535fd06b3e01bdc21 on an m1 mac. No problems building. Started a new node with -v2transport=1 turned on. At different points of IBD, I added/removed peers to make sure I could get blocks from all v1 peers, v1 + v2 peers, v2-only peers. Once the node was synced, connected to only v2 peers and tested broadcasting transactions. Also compared mempool contents and chaintips with a v23 node running the v1 transport. So far, everything works as expected.
  58. jsarenik commented at 4:13 pm on November 2, 2022: none

    Thanks for doing this! I compiled it myself with one little unrelated patch on Alpine Linux (musl-libc) x86_64. It is running at be.anyone.eu.org, all three Bitcoin networks ­— mainnet, testnet and signet (and also Plebnet Playground at explorer.plebnet.fun, which is a custom signet). IPv4, IPv6, Tor, I2P. Feel free to test with my node running on residential 30 Mbps DSL connection 24/7.

    UPDATE: Added IPv6 (via tunnelbroker.net) which did not reliably work before BIP324 and for some time I was assuming that unencrypted Bitcoin traffic on port 8333 is blocked by HE.net, but now it works well. May not be really related to BIP324. On all networks.

    UPDATE2: Peers can come and verify the session IDs (or for signet). The addresses without square brackets and what follows after the closing square bracket are MD5 hashed, and only first 16 bytes of the hash are shown there. Example: solinxtxcc7a2qzs42tjj3zayaxjvisvskptuqcyikwi35kikota.b32.i2p:0 hashes to 992dfda709f41ad9.

  59. dhruv force-pushed on Nov 18, 2022
  60. dhruv commented at 5:19 am on November 18, 2022: contributor

    Latest push:

  61. DrahtBot added the label Needs rebase on Nov 19, 2022
  62. DrahtBot removed the label Needs rebase on Nov 21, 2022
  63. dhruv force-pushed on Nov 24, 2022
  64. dhruv commented at 5:08 am on November 24, 2022: contributor

    Latest push:

    • Bringing in upstream rebase to #26153
    • Fixed a bug where a disconnection after receiving some bytes but prior to the end of the key exchange phase would result in a downgrade when it should not
  65. DrahtBot added the label Needs rebase on Nov 25, 2022
  66. in src/net.h:655 in 8f7714e338 outdated
    652@@ -526,7 +653,9 @@ class CNode
    653           const std::string& addrNameIn,
    654           ConnectionType conn_type_in,
    655           bool inbound_onion,
    656-          CNodeOptions&& node_opts = {});
    657+          CNodeOptions&& node_opts = {},
    658+          bool prefer_p2p_v2 = false,
    659+          TransportProtocolType transport_type_in = TransportProtocolType::V1);
    


    dergoegge commented at 2:45 pm on November 26, 2022:
    These should move to CNodeOptions

    dhruv commented at 5:56 am on January 14, 2023:
    Done.
  67. in src/net.h:735 in 8f7714e338 outdated
    733     std::list<CNetMessage> vRecvMsg; // Used only by SocketHandler thread
    734+    CKey v2_priv_key;
    735+    std::array<std::byte, BIP324_GARBAGE_TERMINATOR_LEN> v2_sent_garbage_terminator;
    736+    std::array<std::byte, BIP324_GARBAGE_TERMINATOR_LEN> v2_recv_garbage_terminator;
    737+    std::vector<std::byte> v2_garbage_bytes_recd;
    738+    bool v2_keys_derived{false};
    


    dergoegge commented at 2:50 pm on November 26, 2022:

    It would make sense bundle all the bip324 state into their own structure or class.

    0std::unique_ptr<BIP324ConnectionState> m_bip324_state;
    
     0struct BIP324ConnectionState {
     1    CKey v2_priv_key;
     2    std::array<std::byte, BIP324_GARBAGE_TERMINATOR_LEN> v2_sent_garbage_terminator;
     3    std::array<std::byte, BIP324_GARBAGE_TERMINATOR_LEN> v2_recv_garbage_terminator;
     4    std::vector<std::byte> v2_garbage_bytes_recd;
     5    bool v2_keys_derived{false};
     6    std::string m_v2_session_id;
     7    EllSwiftPubKey ellswift_pubkey;
     8    std::vector<std::byte> peer_ellswift_buf;
     9    std::atomic_bool v2_key_exchange_complete{false};
    10    std::atomic_bool m_authenticated_v2_garbage{false};
    11    bool v2_garbage_terminated{false};
    12};
    

    dhruv commented at 5:56 am on January 14, 2023:
    Done. I chose to break it down into two different structs – one for private members and one for public ones.
  68. dergoegge changes_requested
  69. dergoegge commented at 3:59 pm on November 26, 2022: member

    Concept ACK - Thanks to everyone working on this!

    My review feedback will be limited to the networking changes as I can’t really judge the soundness of the underlying crypto used (Cryptography 1 by Dan Boneh didn’t quite get me there yet :D).

    The current approach here feels a little “slapped on” to me. I am surprised by all the special casing around handling v2 connections which I think mostly stems from the fact that currently v1 and v2 connections are being accepted on the same port. Is that really required/desirable for some reason?

    I kind of expected to see a similar approach to how we handle connections from different networks (e.g. Tor, CJDNS, I2P). That is, we listen for v2 connection on a dedicated port in a separate thread (or a separate process but that could also be done later or never) that handles all the initial v2 connection setup (i.e. key exchange). Once a v2 connection has been successfully established the thread registers the new connection with the connection manager. This would lead to the changes here being much more self contained and remove the need for the special casing around v2 connection setup.

  70. jsarenik commented at 3:33 pm on December 1, 2022: none

    Updated my nodes to 8f7714e33804b6cf96fedbe05751d1a700e1e8d1

    Node1’s addresses (no Tor anymore) are visible via node-details.

    Node2’s addresses are:

    • Tor: 3xept3fmslplcm4hu4gnbpquz3d5twqg2m5hqeimbvuzykiuxan5kgad.onion
    • Yggdrasil (which thinks it is IPv6): 200:40c3:3ffe:680:a5b2:9d41:3928:fb73
  71. jsarenik commented at 4:03 pm on December 4, 2022: none

    My review feedback will be limited to the networking changes as I can’t really judge the soundness of the underlying crypto used (Cryptography 1 by Dan Boneh didn’t quite get me there yet :D).

    Much appreciated. I did not even finish that first course yet :)

    The current approach here feels a little “slapped on” to me. I am surprised by all the special casing around handling v2 connections which I think mostly stems from the fact that currently v1 and v2 connections are being accepted on the same port. Is that really required/desirable for some reason?

    Practically speaking for myself, it actually only makes sense when v1 and v2 live on the same port. Example: a tunelled IPv6 connection may be doing some L7 blocking, but when the filter sees other full-duplex communication which it can not decipher, it may be less willing to block the whole port since there is other noise on it.

    I kind of expected to see a similar approach to how we handle connections from different networks (e.g. Tor, CJDNS, I2P). …

    Tor, CJDNS and I2P are both layers and addressing schemes at the same time. CJDNS (and Yggdrasil) are merely subsets of IPv6 address space for addressing, but they do their own thing encrypting the contents of the packets.

    V2 IMHO makes sense on clearnet IPv4 or IPv6. Even if the other transports go “cleartext” on the user-level, still no one can see anything during the transfer and V2 will not make it any better, just use more CPU for nothing (double-encryption, though useful for testing by being able to add an onion address as a V2 peer).

  72. sipa commented at 4:26 pm on December 4, 2022: member
  73. dhruv closed this on Dec 15, 2022

  74. dhruv deleted the branch on Dec 15, 2022
  75. dhruv reopened this on Dec 15, 2022

  76. dhruv force-pushed on Dec 15, 2022
  77. dhruv commented at 5:06 pm on December 15, 2022: contributor

    Latest push:

    • Rebased upstream changes
    • New rekey ratchet mechanism (forward security) based on ChaCha20 instead of SHA256. BIP changes are WIP and coming soon.

    Note for users running mainnet nodes for testing: This is a breaking change in the protocol. So you will need to update code to peer with nodes that have updates.

  78. DrahtBot removed the label Needs rebase on Dec 15, 2022
  79. rot13maxi commented at 2:46 pm on December 17, 2022: none
    @dhruv can you elaborate on why the move to chacha20?
  80. dhruv commented at 2:50 pm on December 17, 2022: contributor
    @rot13maxi It’s a minor improvement: ChaCha20 is cheaper than SHA256 and we are already using it (for the BIP324 suite, it reduces a dependency). For the length encryption, it can be done without even initializing a new instance. The payload encryption is also already using RFC8439 ChaCha20-Poly1305.
  81. DrahtBot added the label Needs rebase on Dec 19, 2022
  82. dhruv force-pushed on Jan 14, 2023
  83. dhruv commented at 5:57 am on January 14, 2023: contributor

    Latest push:

  84. DrahtBot removed the label Needs rebase on Jan 14, 2023
  85. dhruv force-pushed on Jan 17, 2023
  86. dhruv commented at 6:14 pm on January 17, 2023: contributor

    Latest push:

  87. dhruv force-pushed on Jan 23, 2023
  88. dhruv commented at 10:15 pm on January 23, 2023: contributor
    Rebased changes from #26153
  89. DrahtBot added the label Needs rebase on Jan 28, 2023
  90. dhruv force-pushed on Feb 2, 2023
  91. dhruv commented at 6:29 am on February 2, 2023: contributor
    Rebased. Brought in upstream changes.
  92. DrahtBot removed the label Needs rebase on Feb 2, 2023
  93. DrahtBot added the label Needs rebase on Feb 15, 2023
  94. RFC8439 nonce and counter for ChaCha20 8e04f058a4
  95. RFC8439 implementation and tests 7a9d2fb8cf
  96. Adding forward secure FSChaCha20 acd664e805
  97. dhruv force-pushed on Feb 21, 2023
  98. dhruv commented at 3:36 am on February 21, 2023: contributor
    Rebased.
  99. DrahtBot removed the label Needs rebase on Feb 21, 2023
  100. DrahtBot added the label Needs rebase on Feb 22, 2023
  101. vostrnad commented at 9:03 pm on March 1, 2023: none

    Is there a reason the v2transport option should be off by default? Does it even need to exist at all? I was accidentally running this for a few days with v2 turned off and was confused why nodes advertising P2P_V2 weren’t actually using the v2 protocol. Everything works fine now.

    Also, it would be nice to have the transport_protocol_type and v2_session_id fields displayed in the GUI, although that might possibly belong to a separate PR in the GUI repo (not sure what the development process dictates here).

  102. dhruv force-pushed on Mar 8, 2023
  103. dhruv commented at 3:30 pm on March 8, 2023: contributor
    Rebased with master. Brought in changes to #23233, #23561 and #23432. The changes to the BIP here are implemented and result in a breaking change for everyone running v2 nodes on mainnet.
  104. DrahtBot removed the label Needs rebase on Mar 8, 2023
  105. vostrnad commented at 1:59 am on March 9, 2023: none
    After updating, node is stuck in a loop trying to connect to manually added nodes that haven’t been updated yet, without downgrading to a v1 connection. It might be just fine, but I thought I’d mention it.
  106. dhruv commented at 3:17 pm on March 9, 2023: contributor
    @vostrnad this is expected because the breaking change is in the p2p layer and not the transport. So a v2 connection is successfully established, and likely the inbound peer (not updated) never sees something that it interprets as the VERSION message. It should have timed out/failed eventually though. Did it do that? This isn’t currently considered a downgrade scenario because a v2 transport connection was established. If we downgrade to v1 on p2p unresponsiveness, my thinking is that would lead to downgrades in scenarios where a peer is trying to evict, etc. but doesn’t close the TCP connection cleanly. Open to suggestions, of course.
  107. DrahtBot added the label Needs rebase on Mar 12, 2023
  108. BIP324 Cipher Suite 187761fc8a
  109. Allow for RFC8439 AD in cipher suite interface 8405856ed9
  110. Merge branch 'bip324-cipher-suite' into bip324-net-v2 0c3c7ab105
  111. Add BIP324 short-IDs to protocol.cpp
    Co-authored-by: Jonas Schnelli <dev@jonasschnelli.ch>
    9d9c46f702
  112. Add BIP324 v2 transport serializer and deserializer
    Co-authored-by: Jonas Schnelli <dev@jonasschnelli.ch>
    c32329c47a
  113. fuzz: Add fuzz test for v2 transport {de}serialization 16eeb431eb
  114. Expose BIP324CipherSuite AAD via transport classes f01955687f
  115. Squashed 'src/secp256k1/' changes from bdf39000b9..8034c67a48
    8034c67a48 Add doc/ellswift.md with ElligatorSwift explanation
    e90aa4e62e Add ellswift testing to CI
    131faedd8a Add ElligatorSwift ctime tests
    198a04c058 Add tests for ElligatorSwift
    9984bfe476 Add ElligatorSwift benchmarks
    f053da3ab7 Add ellswift module implementing ElligatorSwift
    76c64be237 Add functions to test if X coordinate is valid
    aff948fca2 Add benchmark for key generation
    5ed9314d6d Add exhaustive tests for ecmult_const_xonly
    b69fe88d5e Add x-only ecmult_const version for x=n/d
    427bc3cdcf Merge bitcoin-core/secp256k1#1236: Update comment for secp256k1_modinv32_inv256
    647f0a5cb1 Update comment for secp256k1_modinv32_inv256
    5658209459 Merge bitcoin-core/secp256k1#1228: release cleanup: bump version after 0.3.0
    28e63f7ea7 release cleanup: bump version after 0.3.0
    
    git-subtree-dir: src/secp256k1
    git-subtree-split: 8034c67a48dc1334bc74ee4ba239111a23d9789e
    963f9a5c61
  116. Merge commit '963f9a5c6159d985eb16b115a7d27027074827ed' into bip324-ellsq 1f0eca0651
  117. Encode CKey to ElligatorSwift representation 13423e6054
  118. Bench tests for CKey->EllSwift 697a23765e
  119. Fuzz tests for CKey->EllSwift ae4b695aac
  120. Merge branch 'bip324-cipher-suite' into bip324-handshake ea3e911558
  121. Squashed 'src/secp256k1/' changes from bdf39000b9..8034c67a48
    8034c67a48 Add doc/ellswift.md with ElligatorSwift explanation
    e90aa4e62e Add ellswift testing to CI
    131faedd8a Add ElligatorSwift ctime tests
    198a04c058 Add tests for ElligatorSwift
    9984bfe476 Add ElligatorSwift benchmarks
    f053da3ab7 Add ellswift module implementing ElligatorSwift
    76c64be237 Add functions to test if X coordinate is valid
    aff948fca2 Add benchmark for key generation
    5ed9314d6d Add exhaustive tests for ecmult_const_xonly
    b69fe88d5e Add x-only ecmult_const version for x=n/d
    427bc3cdcf Merge bitcoin-core/secp256k1#1236: Update comment for secp256k1_modinv32_inv256
    647f0a5cb1 Update comment for secp256k1_modinv32_inv256
    5658209459 Merge bitcoin-core/secp256k1#1228: release cleanup: bump version after 0.3.0
    28e63f7ea7 release cleanup: bump version after 0.3.0
    
    git-subtree-dir: src/secp256k1
    git-subtree-split: 8034c67a48dc1334bc74ee4ba239111a23d9789e
    984b3d0a1f
  122. Merge commit '984b3d0a1faabc1657e0dd33432fdadfd78335a9' into bip324-handshake 007a92ed80
  123. Enable ECDH computation on secp256k1 keys 3d1b276949
  124. Bench test for ECDH 4b7be855dd
  125. Fuzz test for ECDH 6ffa68f4db
  126. HKDF key derivation from ECDH secret for BIP324 dc2527fb2f
  127. Fuzz test for BIP324 key derivation 178ef757ba
  128. Merge branch 'bip324-handshake' into bip324-enable 88ae72f9aa
  129. Merge branch 'bip324-ellsq' into bip324-enable 72bdb8b7bd
  130. scripted-diff: rename use_v2 to use_addr_v2 in src/protocol.h
    -BEGIN VERIFY SCRIPT-
    sed -i 's/use_v2/use_addr_v2/g' ./src/protocol.h
    -END VERIFY SCRIPT-
    8cd437261e
  131. p2p: Advertise v2 transport if CLI arg is on a5a83366a7
  132. rpc: addnode arg to use BIP324 v2 p2p 7def2aeaba
  133. refactor: Add InitP2P() to NetEventsInterface ac42744a44
  134. p2p: Use v2 transport between supportive peers 686c2063de
  135. p2p: BIP324 transport version messages f2a0cb9b5b
  136. p2p: BIP324 shapable key exchange 7d205b30f6
  137. p2p: BIP324 v2.0 clients retry with v1 protocol a1c678affd
  138. test: Functional test for opportunistic encryption 0a3d926510
  139. test: BIP324 test vectors f52afc632c
  140. rpc: Expose transport type via getpeerinfo ae3acd187e
  141. rpc: Expose BIP324 session id via getpeerinfo 77f7da1cad
  142. dhruv force-pushed on Mar 21, 2023
  143. dhruv commented at 4:04 am on March 21, 2023: contributor
    Rebased.
  144. DrahtBot removed the label Needs rebase on Mar 21, 2023
  145. DrahtBot added the label Needs rebase on Mar 23, 2023
  146. DrahtBot commented at 7:20 pm on March 23, 2023: contributor

    🐙 This pull request conflicts with the target branch and needs rebase.

  147. fanquake commented at 11:14 am on May 6, 2023: member
    Closing for now. This will be picked up again later. BIP324 review attention should be directed towards #27479 and https://github.com/bitcoin-core/secp256k1/pull/1129.
  148. fanquake closed this on May 6, 2023

  149. sipa referenced this in commit 4ff7d8f3bb on Aug 29, 2023
  150. bitcoin locked this on Sep 14, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 15:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me