Net: CNetAddr: add support to (un)serialize as ADDRv2 (p2p)

https://github.com/bitcoin/bitcoin/pulls/19845

Host: jonatack  -  PR authors: vasild , dongcarl

The PR branch HEAD was 3eef1e6 at the time of this review club meeting.

Notes

Motivation

  • Today’s PR 19845 is part of PR 19031, a proposed implementation of the BIP155 addrv2 message.

  • addrv2 is a new p2p message format proposed in early 2019 by Wladimir J. van der Laan to support networks that have longer endpoint addresses than fit in the 128 bits/16 bytes of the current addr message.

  • Issue 18884 “Tor v3 support” is the meta-issue for tracking overall progress on BIP155 and its implementation.

  • This work is urgent because Tor v2 was deprecated yesterday (September 15, 2020) with 0.4.4.x and will be obsoleted in 0.4.6.x (July 15, 2021). Tor v2 is expected to be completely disabled in Tor client stable versions on October 15, 2021 as announced here: “From today (July 2nd, 2020), the Internet has around 16 months to migrate from onion services v2 to v3 once and for all.”

  • Why is Tor v2 EOL? According to this Tor mailing list post: “To very quickly summarize why we are deprecating, in one word: Safety. Onion service v2 uses RSA1024 and 80 bit SHA1 (truncated) addresses. It also still uses the TAP handshake which has been entirely removed from Tor for many years now except v2 services. Its simplistic directory system exposes it to a variety of enumeration and location-prediction attacks that give HSDir relays too much power to enumerate or even block v2 services. Finally, v2 services are not being developed nor maintained anymore. Only the most severe security issues are being addressed.”

  • A previous review club meeting discussing the parent PR 19031 provides more background and context.

Network address types newly recognized by PR 19845

  • Next-generation Tor v3 Onion addresses - “Better crypto (replaced SHA1/DH/RSA1024 with SHA3/ed25519/curve25519), improved directory protocol leaking much less information to directory servers, a smaller surface for targeted attacks, better onion address security against impersonation, and a more extensible introduction/rendezvous protocol.”

  • Invisible Internet Project (I2P) - “The I2P network provides strong privacy protections for communication over the Internet. Many activities that would risk your privacy on the public Internet can be conducted anonymously inside I2P.”

  • Cjdns - “Cjdns implements an encrypted IPv6 network using public-key cryptography for address allocation and a distributed hash table for routing. This provides near-zero-configuration networking, and prevents many of the security and scalability issues that plague existing networks.”

  • With PR 19845, you can make peer connections to these network address types by using the addnode RPC or the -addnode / -connect configuration options. Here’s how:
    • ./src/bitcoin-cli help addnode
    • ./src/bitcoind -h | grep -A4 "\-addnode=\|\-connect="
  • For rumouring these address types, more is needed. For instance, PR 19954—the remaining step of 19031—enables running bitcoind as a Tor v3 service.

Technical Notes

  • Today’s PR 19845 adds the ability to serialize and deserialize internet addresses in addrv2 format. It also adds Tor v3 address parsing and builds on just-merged PR 19841, “Implement Keccak and SHA3_256”. The latter is needed for Tor v3 support, as the conversion from BIP155 encoding to .onion notation uses an SHA3-based checksum.

  • The addrv2 feature is enabled by OR-ing an integer flag, ADDRV2_FORMAT (code), into the stream version.

  • The PR makes use of the custom Span type, which was introduced into Bitcoin Core in PR 12886 and represents a vector-like view to a range of contiguous elements in memory analogous to std::span in C++20. If you are not familiar with Span, it was discussed in a recent review club meeting.

  • Tor v3 address parsing should follow this spec, which is also summarized here in the BIP.

Questions

  1. Warm-up question #1: Visually, how can you tell the difference between a Tor v2 and v3 address?

  2. Warm-up question #2: List all the network address types that Bitcoin Core can support after this PR. What is the size in bytes (address length) for each of them? What is the maximum address length Bitcoin Core will be able to support?

  3. Warm-up question #3: Is addrv2 deserialization faster or slower than v1? By roughly how much?

  4. Did you review the PR? Bonus: Did anyone also review PR 19841, “Implement Keccak and SHA3_256”?

  5. What steps did you take to review this PR? Did you review on GitHub, or in your local dev environment? Did you review commit-by-commit, or the whole diff? What did you review first: the code, the tests, or BIP155? Did you verify that the code corresponds to the spec for each network address type? Any thoughts on the test coverage? Do you see anything that is not tested or could also be tested?

  6. When should the new sendaddrv2 message type, aka “send me addrv2”, be sent?

  7. What does this implementation do if an unknown or non-validateable network address type is encountered? How about a known network with wrong length? Are these considered misbehavior? Discouraged? Not relayed? Ignored? Why?

  8. What is meant by “variable-length addresses” in BIP155 and when referring to addrv2?

  9. How will the new address formats be saved in peers.dat, which currently persists addresses in 16-byte IPv6 format?

  10. Why was the boolean pad parameter added to EncodeBase32()?

  11. How do you think Bitcoin Core should make the transition from Tor v2 to v3? All at once, or v3 first opt-in, then default (and v2 opt-in or deprecated)? In which releases?

Meeting Log

  119:00 <jonatack> #startmeeting
  219:00 <@jnewbery> hi
  319:00 <jonatack> Hi all! Welcome to this week's episode of the Bitcoin Core PR Review club!
  419:00 <jonatack> #topic This week, we are looking at PR 19845 - "Net: CNetAddr: add support to (un)serialize as ADDRv2" (p2p)
  519:00 <jonatack> Please refer to https://bitcoincore.reviews/19845 that contains notes and questions regarding today's meeting and PR.
  619:00 <urethane> hi
  719:00 <michaelfolkson> hi
  819:00 <gzhao408> hai
  919:00 <brikk> hi
 1019:00 <jonatack> Anyone here for the first time?
 1119:00 <shaunsun> hi
 1219:01 <emzy> hi
 1319:01 <troygiorshev> hi
 1419:01 <jonatack> Let's multithread this and dig in. Warm-up question #1: Visually, how can you tell the difference between a Tor v2 and v3 address?
 1519:01 <dongcarl> hi
 1619:02 <vasild> hi
 1719:02 <jonatack> Warm-up question #2: List all the network address types that Bitcoin Core can support after this PR. What is the size in bytes (address length) for each of them? hint: src/netaddress.h::95-114
 1819:02 <emzy> Lengh of the onion address.
 1919:02 <shaunsun> v3 is longer right?
 2019:02 <pinheadmz> hi!
 2119:02 <sipa> hi.
 2219:02 <urethane> v3 is much longer
 2319:02 <jonatack> emzy: shaunsun: urethane: yes
 2419:02 <michaelfolkson> 32 bytes vs 10 bytes
 2519:02 <jonatack> A tor v2 address is 15 characters in length, not counting the .onion suffix: 57qr3yd1nyntf5k.onion
 2619:02 <jonatack> A tor v3 address is 56 characters: 7zvj7a2imdgkdbg4f2dryd5rgtrn7upivr5eeij4cicjh65pooxeshid.onion
 2719:03 <jonatack> (visually)
 2819:03 <urethane> e.g. openpgp.org v3: zkaan2xfbuxia2wpf7ofnkbz6r5zdbbvxbunvp5g2iebopbfc4
 2919:03 <emzy> ipv4, ipv6, torv2, torv3, I2P, CJDNS.
 3019:03 <urethane> .onion
 3119:03 <sipa> it's that much longer because it's not just the payload going from 10 to 32 bytes, but also a version number and checksum are added
 3219:04 <jonatack> emzy: right, ipv4 4 bytes; ipv6 16 bytes; torv2 10 bytes; torv3 32 bytes; i2p 32 bytes; cjdns 16 bytes
 3319:04 <jonatack> Question 2 *BONUS*: describe any changes going from addrv1 to addrv2, in the size of addrv1-supported addresses, e.g. ipv4, ipv6, torv2?
 3419:04 <vasild> also "internal" is supported :)
 3519:05 <nehan> hi
 3619:05 <sipa> cjdns could be 15 bytes, i think?
 3719:05 <vasild> ah, right!
 3819:05 <pinheadmz> the serialization of all adress types is different in addrv2
 3919:05 <michaelfolkson> Is it padded to make it 16?
 4019:06 <pinheadmz> UnserializeV1Stream() vs UnserializeV2Stream() in netaddress.h
 4119:06 <vasild> hmm, do we want to rely on the fact that they will never change the 1 byte prefix to something else?
 4219:06 <sipa> michaelfolkson: cjdns nominally uses IPv6 addresses in range fc00::/8
 4319:06 <sipa> since the first byte is always 0xfc, we could drop it from relay, as it's already explicitly tagged with the cjdns marker
 4419:06 <sipa> vasild: good question; perhaps not worth it
 4519:07 <jonatack> Anyone wish to describe changes going from addrv1 to addrv2, in the size of addrv1-supported addresses?
 4619:08 <pinheadmz> oh are we not using 16 bytes for ipv4 anymore?
 4719:08 <pinheadmz> because we have more speciifc types instead of trying to use 16 bytes for "everything"
 4819:08 <sipa> pinheadmz: indeed!
 4919:08 <jonatack> no, it's now much smaller
 5019:08 <jonatack> significantly
 5119:08 ⚡ pinheadmz fist bump booyah
 5219:09 <jonatack> ipv4: 16 -> 6 bytes
 5319:09 <jonatack> ipv6: 16 -> 18 bytes
 5419:09 <jonatack> oops, it's bigger, but...
 5519:09 <jonatack> torv2: 16 -> 12 bytes
 5619:09 <jonatack> estimate the bandwidth savings
 5719:10 ⚡ urethane dreams of gigamegs
 5819:10 <urethane> lol
 5919:10 <jonatack> iirc vasild estimated ~ half less bandwidth in a recent irc discussion
 6019:10 <vasild> http://www.erisian.com.au/bitcoin-core-dev/log-2020-09-11.html#l-478
 6119:11 <jonatack> Quick: What is the maximum address length Bitcoin Core will be able to support?
 6219:11 <vasild> hmm, is there a way to dump all addrman database in a human readable form?
 6319:11 <michaelfolkson> 512
 6419:11 <wiz> hmm, I guess it's not possible to fit a v3 onion in a DNS Seed unless we implement TXT records or something eh
 6519:11 <emzy> 512 bytes
 6619:12 <jonatack> michaelfolkson: yes!
 6719:12 <vasild> wiz: right
 6819:12 <michaelfolkson> Which is....huge
 6919:12 <jonatack> wiz: right
 7019:12 <wumpus> that is true, though, DNS seeds are unusable through tor anyhow
 7119:12 <sipa> wiz: indeed, but on the other hand... if you're using tor you probably don't want to use DNS seeds (directly) in the first place (and bitcoin core doesn't)
 7219:13 <wumpus> that ^^
 7319:13 <sipa> instead we ask an exit node to do the resolving, connect to one of the seed's results, send it a getaddr, and disconnect when the response comes back
 7419:13 <sipa> which will transparently keep working for addrv2, and support torv3 just fine
 7519:13 <wumpus> (this is also, in general, the case for other overlay networks)
 7619:14 <jonatack> Question: did you review the PR? (y / n)
 7719:14 <jonatack> y
 7819:14 <wiz> so what's the purpose of the onioncat encoded AAAA records that start with fd87:d87e:eb43::/48 then? totally unused?
 7919:14 <pinheadmz> y
 8019:14 <emzy> sipa: so without clearnet nodes this will not work. But clearnet is not dead yet.
 8119:14 <urethane> wumpus is it "DNS seeds are unusable through tor anyhow" or DNS seeds are ill-advised given user expectations of a node operator using tor?
 8219:14 <sipa> wiz: relay of onion addresses to non-tor nodes
 8319:14 <emzy> y (only tested)
 8419:15 <wiz> okay, so do we still want to support that for v3 onions in some new encoding or TXT records?
 8519:15 <wumpus> urethane: they are not usable through Tor. You can do DNS lookups through Tor (through a SOCKS5 extension), in principle, but not the kind of query that returns multiple results.
 8619:15 <sipa> wiz: that would be a ton of work, i'm afraid
 8719:15 <sipa> (as in: implement our own dns resolver that works over tor...)
 8819:16 <urethane> wumpus ty
 8919:16 <wumpus> urethane: they're also super-unreliable, because it's controlled by exit nodes
 9019:16 <wumpus> wiz: no, I think that's not necessary
 9119:16 <urethane> wumpus that's the security fallibility I thought you were referring to
 9219:16 <troygiorshev> n
 9319:17 <wumpus> emzy: it works without clearnet nodes; in that case it would rely on the hardcoded onion seeds in the source code though
 9419:18 <wumpus> (which is okay, some are just as reliable as the DNS seeds)
 9519:18 <shaunsun> partial y (was able to connect to v3 addresses)
 9619:18 <jonatack> shaunsun: nice
 9719:19 <pinheadmz> doesn't tor work with some kind of directory nodes to help route?
 9819:19 <emzy> wumpus: I will make a mental note to set one up then.
 9919:19 <pinheadmz> like isnt there a tor version of DNS in that sense
10019:20 <sipa> pinheadmz: yes, but for hidden services and relays (I think) - not DNS
10119:20 <wumpus> correct, Tor doesn't use names (for anything besides routing through exit nodes), only public keys
10219:20 <pinheadmz> sure bc theres no IPs in tor anyway really - but see what im getting at? could there be a bitcoin core tor directory that served bootstrapping nodes over tor?
10319:21 <pinheadmz> er, i guess thats the same as just connecting to a seed node over tor and getting addr messages
10419:21 <emzy> pinheadmz: I think you can see a hardcodes onion seed as a dnsseed for tor.
10519:21 <wumpus> but why? any node can give you other peers, it's decentralized on purpose
10619:21 <pinheadmz> yeah
10719:22 <jonatack> pinheadmz: "directory" node... just to recap, irc there are 3 basic types of tor nodes: bridge/gate, relays and exit; often a tor node has more than one of those roles
10819:22 <urethane> pinheadmz "some kind of directory nodes to help route?" I think you're referring to what's called HSDirs in tor
10919:22 <michaelfolkson> Won't it take a while to find a node that gives you Bitcoin peers? Connect to loads of Tor nodes that aren't interested in Bitcoin
11019:23 <sipa> pinheadmz: "could there be a bitcoin core tor directory that served bootstapping nodes over tor?" -> that's called a seed node
11119:23 <sipa> and it needs literally zero modifications to work :)
11219:23 <pinheadmz> yep I realize i just circled back to what we already ahve
11319:23 <vasild> contrib/seeds/nodes_main.txt
11419:23 <wumpus> michaelfolkson: it will only connect to (supposed) bitcoin peers which will give you other bitcoin peers
11519:24 <urethane> tor v2 doesn't have privacy for onion addresses from an HSDirs observer, tor v3 does have onion service privacy from HSDirs
11619:24 <wumpus> torv3 definitely improved things in that regard
11719:25 <urethane> for that reason alone, users should stop using v2 :)
11819:25 <vasild> they will be forced to stop using v2 very soon
11919:25 <vasild> in less than a year
12019:26 <jonatack> snooping of HSDir relays is improved by v3? TIL
12119:26 <michaelfolkson> Ok yeah. But you need to find (supposed) Bitcoin peers in the first place. Lots of trial and error pinging?
12219:26 <wumpus> michaelfolkson: that's what the hardcoded peers are for, which are updated every major version
12319:27 <wumpus> michaelfolkson: it will never ping random addresses to find bitcoin peers, never :)
12419:27 <michaelfolkson> Ok thanks
12519:27 <sipa> and the DNS seeds
12619:27 <jonatack> michaelfolkson: if you have some good addnode peers in your conf file, maybe seednode too, it makes a big difference afaict
12719:28 <wumpus> you could potentially do that in 32 bit IPv4, even thn it'd likely take a long time, but it would be pointless with 32 byte addresses
12819:28 <emzy> hehe
12919:28 <urethane> list_of_every_ip_address.txt
13019:29 <jonatack> I'm not sure there's much point in going through the "how did you review" questions
13119:30 <jonatack> I did like the tests vasild added
13219:30 <michaelfolkson> And Sjors' test suggestions
13319:31 <vasild> I added two more (not pushed yet), guided by code coverage (seeing which line is not covered and writing test for it)
13419:31 <wumpus> I only tested (have two running Torv3 nodes now) and did very light review FWIW
13519:31 <michaelfolkson> What do you use for code coverage? Marco's site?
13619:31 <jonatack> indeed, I think some addrv1 unit tests could be added to have equivalent coverage
13719:32 <jonatack> michaelfolkson: i think vasild uses his own tool for that... vasild?
13819:32 <wumpus> your code works on ARM Linux and FreeBSD x86_64
13919:32 <michaelfolkson> https://marcofalke.github.io/btc_cov/
14019:32 <vasild> michaelfolkson: no, I run clang's tools and then a script to hilight which lines in the coverage report have been modified by the patch
14119:33 <vasild> https://github.com/vasild/filter_coverage
14219:33 <michaelfolkson> Cool, thanks
14319:33 <vasild> if some of travis or cirus can publish build products that can be integrated in CI
14419:33 <jonatack> Q: When should the new sendaddrv2 message type, aka "send me addrv2", be sent?
14519:34 <vasild> I mean - if during build we create coverage.html that can be browsed later
14619:34 <pinheadmz> I modified a plugin for sublime text for better side-by-side diff'ing: https://github.com/CJTozer/SublimeDiffView/pull/73
14719:34 <brikk> i looked at adding a test for the padding=false case
14819:34 <pinheadmz> makes it more like github side-by-side with the benefit of being able to scroll up and down past blobs of diff
14919:35 <vasild> pinheadmz: and runs locally (I guess)! :)
15019:35 <pinheadmz> yeah i always pull the branch
15119:35 <jonatack> pinheadmz: i'm afraid you lost me with "more like GitHub"
15219:35 <pinheadmz> mmm, in github "files changed" view, in side-by-side mode
15319:36 <vasild> jonatack: that is a good question :)
15419:36 <jonatack> vasild: which one, sendaddrv2 message?
15519:36 <pinheadmz> thats how i like to review. but on github it only shows you the blbos that are different, i prefer to be able to scroll up and down the file (witihout clickthe expansion buttons)
15619:36 <vasild> as early as possible :)
15719:36 <vasild> yes, sendaddr
15819:36 <vasild> v2
15919:37 <jonatack> vasild: i thought troygiorshev wrote an interesting comment on that here https://github.com/bitcoin/bips/pull/907/files#r476667308
16019:37 <jonatack> saying "sendaddrv2 SHOULD be sent after receiving the verack message from the peer" wasn't quite precise enough
16119:38 <vasild> but we can't send it early enough to get the addrfrom in v2 format :(
16219:38 <michaelfolkson> Yeah good comment
16319:39 <jonatack> vasild: hmmmmm
16419:39 <vasild> addrfrom which is part of the version message
16519:39 <wumpus> jonatack: don't know why it's not precise enough; you can't quote say "before any other messages" because other BIPs might also introduce these kind of upgrade messages
16619:40 <sipa> vasild: i think addrfrom is effectively unused?
16719:40 <michaelfolkson> But Troy's point I think is to be clear on whether that means immediately after or at any point in time after
16819:40 <wumpus> it's unused and always send as 127.0.0.1
16919:40 <wumpus> only addrto is used
17019:40 <wumpus> michaelfolkson: as I understood it, it can be any time after
17119:41 <vasild> sipa: hm, right, it is just read from the stream and then discarded, even better!
17219:41 <wumpus> michaelfolkson: there are no defined phases after VERACK
17319:41 <michaelfolkson> Yup. So Troy wanted the comment to make that clear (that's my reading of it anyway)
17419:42 <sipa> oh, but addrMe is used
17519:42 <sipa> that's annoying
17619:42 <sipa> it'
17719:42 <troygiorshev> michaelfolkson: yup that's it!
17819:42 <sipa> it's used to classify our local addresses
17919:42 <wumpus> yes, that one is used
18019:43 <jonatack> michaelfolkson: right. i just sort of wonder, "should", as opposed to what
18119:43 <sipa> so... should BIP155 define a replacement?
18219:43 <troygiorshev> I'm more looking to clarify (or specify) that there are no phases after VERACK.
18319:43 <wumpus> though many implementations just send it as 127.0.0.1 no matter what
18419:44 <wumpus> sipa: I thought about it but really didn't want to change the version message
18519:44 <sipa> wumpus: i mean, the sendaddrv2 message or whatever could contain "oh btw, the address i used to connect to you is X (in addrv2 serialization)"
18619:44 <wumpus> (and I'm happy about that because even changing the network version was super controversial)
18719:45 <sipa> and the addrMe field in version would become a dummy
18819:45 <wumpus> sipa: that would be possible yes
18919:47 <wumpus> it definitely couldn't hurt
19019:47 <vasild> makes sense to me, that would need some adjustment to the BIP and the last commit in https://github.com/bitcoin/bitcoin/pull/19031/commits/a1b067e0 "net: advertise support for ADDRv2 via new message"
19119:48 <vasild> there was some other proposal to add info to the sendaddrv2... what was it...
19219:48 <wumpus> whether a node partakes in addr broadcasting at all
19319:48 <sipa> heh the current BIP doesn't even include sendaddrv2?
19419:48 <wumpus> gmaxwell's proposal
19519:48 <vasild> right!
19619:48 <wumpus> sipa: it's in vasild 's update PR
19719:48 <jonatack> others: i think we are talking about PushNodeVersion() in net_processing.cpp
19819:49 <wumpus> at some point we decided only the merge the updates to the BIP when the implementation was finalized
19919:49 <vasild> sipa: that is in https://github.com/bitcoin/bips/pull/907
20019:49 <jonatack> BIP update PR: https://github.com/bitcoin/bips/pull/907
20119:50 <vasild> and another one: https://github.com/bitcoin/bips/pull/967 :)
20219:50 <wumpus> why are there more than one?
20319:51 <jonatack> thanks, i had not seen PR 967
20419:51 <wumpus> I mean, I think it's easier to have the proposed version of the BIP in one place
20519:51 <sipa> you should ask luke-jr to merge those
20619:51 <sipa> future changes can always be new PRs
20719:51 <wumpus> well some other people claimed it would be better to merge only when things were sure/decided in the implementation
20819:51 <sipa> i have been commenting on #907, not realizing it was a PR with a lot of changes in it already
20919:52 <wumpus> don't have a strong opinion about it
21019:52 <sipa> oh, ok
21119:52 <vasild> 907 started being a moving target, invalidating ACKs all the time, so I decided to not update it with the stuff from 967, also 907 contains mods which have been agreed on. While 967 has got zero discussion so far. I did not want to add some controversal stuff to the settled 907
21219:52 <wumpus> but yes, having multiple PRs is really confusing here
21319:52 <sipa> well, there is a BIP - it's confusing if there are multiple places where the currently-discussed version is held
21419:52 <sipa> it has a number already; ideally that number means just one thing
21519:52 <sipa> (or at least, one thing at any given point in time)
21619:53 <wumpus> but having a lot fo small merges to the BIP wouldn't be great either
21719:53 <michaelfolkson> Why does it matter?
21819:53 <wumpus> it clutters the BIP repostiory
21919:53 <wumpus> it's not meant for incremental development
22019:54 <wumpus> you can have have the in-progress BIP somewhere else
22119:54 <urethane> increases the mental cost of review and discussion
22219:54 <sipa> vasild: fwiw, whether a BIP change gets merged is solely up to the author(s) and editor
22319:54 <sipa> you don't need acks from anyone
22419:54 <michaelfolkson> Hmm I'd have thought it was inevitable in cases such as this. Changes invalidate previous ACKs etc
22519:54 <jonatack> ISTM the BIP has different versions as well: https://github.com/bitcoin/bips/blob/9286b5254317d9e73fb25c5f0acd2b2d9937843e/bip-0155.mediawiki vs https://github.com/bitcoin/bips/blob/master/bip-0155.mediawiki
22619:54 <vasild> sipa: :-O
22719:54 <jonatack> i've been reading the first one
22819:55 <sipa> it's a proposal, you decide what the proposal is
22919:55 <wumpus> it's nice to have ACKs from other people though
23019:55 <sipa> wumpus: since you're the author, you should ack these changes ;)
23119:55 <sipa> (or not)
23219:56 <wumpus> I have (but not openly I guess)
23319:56 <wumpus> I've always discussed with vasild about this
23419:56 <sipa> of course
23519:56 <michaelfolkson> Ok understood. So it is up to the author (vasild) to ask Luke to merge changes whenever vasild is happy with them
23619:56 <sipa> michaelfolkson: wumpus, actually
23719:56 <sipa> just saying - luke-jr's is probably just waiting for wumpus to ACK them
23819:56 <vasild> I am the author of the PR, but not the BIP
23919:56 <michaelfolkson> Ah yes sorry
24019:57 <urethane> 3 minutes left :)
24119:57 <wumpus> yes, I'll ACK them, np
24219:58 <wumpus> as said, I had the impression that everything was in one PR, which tracked the new version of the BIP, which we'd merge when there was agreement on the implementation
24319:58 <michaelfolkson> Did we cover all of Jon's questions?
24419:58 <jonatack> PRs which also pick up work by dongcarl as well, and sipa for the crypto... it's a team effort
24519:58 <wumpus> exactly because all kind of things come up during this
24619:58 <vasild> sendaddrv2(your address is X, I promise to participate to gossip (but I may lie to you))
24719:58 <jonatack> michaelfolkson: no, but the discussion was worth it
24819:58 <michaelfolkson> Haha we did BAD
24919:59 <jonatack> michaelfolkson: the variable-length question was a bit of a trick question
25019:59 <jonatack> to check understanding
25119:59 <jonatack> The addresses defined in BIP155 are of fixed size.
25219:59 <jonatack> We don't actually handle any "variable-length" ones, just addresses of varying fixed lengths.
25319:59 <jonatack> Why was the boolean pad parameter added to EncodeBase32()?
25420:00 <jonatack> this seemed interesting
25520:00 <jonatack> neither vasild nor i know
25620:00 <jonatack> According to vasild: "that was ugly :/ but needed as some === signs appeared at the end of i2p addresses"
25720:00 <brikk> thats how i see it too
25820:00 <michaelfolkson> Hmm
25920:00 <sipa> i probably implemented some standard base32 format, which includes the requirement to pad with = symbols
26020:00 <brikk> but will there be side effects ?
26120:00 <jonatack> for now, it's important to have the code recognize i2p addresses with the correct length and gossip them
26220:00 <sipa> and I2P uses a slightly different standard that omits the padding
26320:00 <jonatack> so that when we add full i2p support, assuming it happens later on,
26420:00 <jonatack> then previous releases (beginning with the next one containing this code) can gossip i2p addresses
26520:01 <brikk> i looked into a test for it to make sure it behaves, because there was none that I could find
26620:01 <vasild> I guess the decode must be prepare to see non-multiple-of-8 input and pad it itself with = before decoding (once we start parsing i2p addresses)
26720:01 <jonatack> brikk: great
26820:01 <wumpus> to be clear, I2P support is not a goal for the initial PR
26920:02 <jonatack> right, for later on
27020:02 <jonatack> BHow do you think Bitcoin Core should make the transition from Tor v2 to v3? All at once, or v3 first opt-in, then default (and v2 opt-in or deprecated)?
27120:02 <sipa> i don't think we should relay I2P addresses before there is support
27220:02 <wumpus> we'd like torv3 support as soon as possible, I2P would be nice but can be done later if the gorundwork is done
27320:02 <brikk> so is the pr doing a bit too much at this point?
27420:02 <jonatack> Should we just bump to v3, or go thru first v3 opt-in, v2 default...
27520:02 <wumpus> jonatack: first both, imo
27620:03 <michaelfolkson> jonatack: So definitely not all at once. I'd say opt-in and then default
27720:03 <sipa> brikk: the BIP defines I2P, so it makes sense to include the BIP-defined checks for it
27820:03 <wumpus> jonatack: opt in is not necessary imo
27920:03 <michaelfolkson> Oh
28020:03 <sipa> that doesn't mean Bitcoin Core needs to relay them
28120:03 <brikk> sipa: i see
28220:03 <jonatack> #endmeeting
28320:03 <wumpus> jonatack: there should be no reason why you'd not want v3 but want v2, but I can see a reason for keeping the v2 open for now, for older nodes
28420:03 <urethane> I don't see the motivation to make it opt-in
28520:03 <wumpus> sipa: right
28620:04 <jonatack> wumpus: right
28720:04 <vasild> https://bitnodes.io/nodes/ -- 22% run the latest version
28820:04 <michaelfolkson> So v2 would be deprecated at some point but a long time in future? Or never deprecated?
28920:04 <@jnewbery> thanks jonatack! Great meeting. I didn't say much, but I found the discussion really interesting :)
29020:04 <jonatack> we have 4 weeks to FF
29120:04 <jonatack> thanks jnewbery!
29220:04 <michaelfolkson> Thanks jonatack!
29320:04 <wumpus> michaelfolkson: after Tor removes it from their codebase, we should too, I think
29420:04 <vasild> michaelfolkson: tor itself will drop v2 in about a year https://blog.torproject.org/v2-deprecation-timeline
29520:04 <troygiorshev> thanks jonatack!
29620:05 <wumpus> thanks jonatack
29720:05 <michaelfolkson> But people could be running old version of Tor?
29820:05 <jonatack> please review the PR everyone :)
29920:05 <emzy> Thanks jonatack!
30020:05 <michaelfolkson> (even when Tor removes it)
30120:05 <wumpus> michaelfolkson: uhm, Tor versions that don't support v3 are ancient by now, and actively dangerous to run
30220:05 <michaelfolkson> Ok makes sense
30320:05 <wumpus> they've supported it for a long time
30420:06 <vasild> michaelfolkson: hmm, given that tor relays through some random tor nodes, I don't know what will happen if you run an old tor node that insists on doing v2 stuff and newer ones have dropped support for it
30520:06 <sipa> torv3 is supported as of 0.3.2.9, released in january 2018
30620:06 <sipa> that's not _that_ long ago
30720:07 <wumpus> for Tor that's really long ago
30820:07 <sipa> i guess :)
30920:07 <wumpus> a lot happens there
31020:07 <wumpus> it's not unlike Bitcoin in that regard
31120:07 <urethane> tor v2 depreciation time line https://blog.torproject.org/v2-deprecation-timeline
31220:07 <vasild> I guess it may likely be impossible to reach torv2 services after https://blog.torproject.org/v2-deprecation-timeline
31320:07 <jonatack> s/it's important to have the code recognize i2p addresses with the correct length and gossip them/just recognize them/
31420:07 <sipa> vasild: i think so
31520:08 <urethane> July 15th, 2021 0.4.6.x: Tor will no longer support v2 and support will be removed from the code base.
31620:08 <urethane> October 15th, 2021 We will release new Tor client stable versions for all supported series that will disable v2.
31720:08 <wumpus> in any case if you're still running those Tor versions by the time addrv2 is removed (and I suppose, the directories and such shut down), they won't be able to use hidden services at all I think
31820:09 <sipa> i think the bigger question is when directory services etc will stop supporting v2
31920:09 <vasild> sipa: my take on "gossip i2p in 0.21 is that, because only 22% run latest version, when we add i2p supprt in e.g. 0.22 then old nodes (0.21) will also gossip it
32020:09 <wumpus> but I don't think bitcoind needs to support pre-addrv3 Tor
32120:10 <sipa> vasild: i'm not sure that's a good idea, as nothing in the network can determine the existance of such addresses...
32220:10 <vasild> sipa: worried about junk traffic?
32320:10 <sipa> so little ways of purging garbage i2p someone may start rumouring
32420:10 <sipa> more worried about garbage in addrmans
32520:10 <vasild> hmm
32620:11 <sipa> i'd say first start gossipping it only between i2p peers, which actually have i2p connectivity
32720:11 <vasild> same with torv2/torv3 addresses for a node that has no tor connectivity. Or IPv6 addresses for a node that has only IPv4 connectivity...
32820:12 <sipa> once there is a tangible network, anything can start gossipping it, as it's useful to prevent partitions
32920:12 <sipa> if there was a concrete plan to have i2p proxy support in the near future, maybe that could be different
33020:14 <wumpus> i'd say first start gossipping it only between i2p peers, which actually have i2p connectivity <- agree
33120:14 <jonatack> seems sensible
33220:15 <sipa> for torv3 i think we expect a torv3-reachable set of bitcoind nodes to appear almost immediately
33320:15 <wumpus> you could say so :)
33420:16 <vasild> currently we gossip addresses from unreachable networks to just 1 peer (and addresses from reachable networks to 2 peers)
33520:16 <sipa> vasild: 1.5!
33620:16 <vasild> did it get merged?
33720:16 <sipa> https://github.com/bitcoin/bitcoin/pull/19728 yup
33820:17 <vasild> yes, https://github.com/bitcoin/bitcoin/pull/19728
33920:17 <sipa> so i think that for "not known to exist" networks that number should probably be even lower, or 0
34020:17 <vasild> so you propose to extend that logic to reachable: 2, unreachable-non-i2p 1.5, unreachable-i2p: 0
34120:18 <sipa> same with cjdns
34220:18 <vasild> unreachable-ipv4-ipv6-tor: 1.5
34320:19 <vasild> reachable: 2, unreachable-ipv4-ipv6-torv2-torv3: 1.5, unreachable-anything-else: 0
34420:22 <vasild> sipa: "more worried about garbage in addrmans" currently for a node to be able to protect itself from this it must have connectivity to all 3: ip4, ip6 and tor, right?
34520:24 <sipa> assuming some reasonable number of honest nodes, it's not that black and white
34620:24 <sipa> as ipv6-capable nodes will learn about useless ipv6 addresses being rumoured, and prune them
34720:25 <sipa> hmm, maybe this isn't that big of a concern
34820:25 <sipa> it just feels strange to me to permit addrman space to be taken up by addresses in a network we know doesn't ezist
34920:26 <vasild> I will sleep over with this, maybe limit size in addrman also based on reachable/unreachable
35020:36 <jonatack> michaelfolkson: thank you for adding to the bitcoinstackexchange answer! just saw that you mentioned it above
35120:36 <sipa> he's been adding many :)
35220:36 <urethane> +1