multi: implement BIP-155 addrv2 support #1812
Conversation
Pull Request Test Coverage Report for Build 1894615664
💛 - Coveralls |
There was a problem hiding this comment.
Great work! Completed an initial pass, and will start to run this using btcd and neutrino on testnet
My main question is regarding backwards compatibility: are we still able to read the old addr v1 JSOn messages? As a follow up: will we convert all the addrv1 to addrv2 on disk when we first start using this PR?
| for { | ||
| remoteMsg, _, err := p.readMessage(wire.LatestEncoding) | ||
| if err == wire.ErrUnknownMessage { | ||
| continue |
There was a problem hiding this comment.
remoteMsg is nil here, would need to bubble up the command string. Could also log it at a lower level inside one of the readMessage helper functions
Crypt-iQ
force-pushed
the
btcd_addrv2
branch
from
746021c
to
cb6f21b
Compare
|
Giving this a spin on mainnet+testnet now. |
|
Attempted to do a diff of my |
|
cc @ellemouton |
|
|
||
| // maxNetAddressV2Payload returns the max payload size for an address used in | ||
| // the addrv2 message. | ||
| func maxNetAddressV2Payload() uint32 { |
There was a problem hiding this comment.
any reason not to have a hardcoded const for this?
There was a problem hiding this comment.
nope, can change, just wanted to show the derivation
| legacyNa.IP = a.addr[:] | ||
| case *torv2Addr: | ||
| legacyNa.IP = a.onionCatEncoding() | ||
| case *torv3Addr: |
There was a problem hiding this comment.
should this not just be the default case so that we catch future network ID types too?
There was a problem hiding this comment.
I think it could just be updated when BIP155.2 comes out? We already ignore unknown network IDs from our peers, so this should only be called for netIDs we know about
|
|
||
| type ipv4Addr struct { | ||
| addr [ipv4Size]byte | ||
| netID networkID |
There was a problem hiding this comment.
since network ID is fixed per addr type, should we not either remove this field and have the Network() func return a const or add a newIPV4Addr(addr) func that sets the netID to a const?
There was a problem hiding this comment.
that also avoids us having to do
addr := &ipv4Addr{}
addr.netID = ipv4
etc in the NetAddressV2FromBytes func
There was a problem hiding this comment.
same could likely go for the addr size
There was a problem hiding this comment.
Network() returns a string, and is only provided to make the structs conform to the net.Addr interface, so we'd have to convert back and forth to an int which I wanted to avoid. I think a newIPV4Addr and related constructors might work, but there may have been a reason I'm forgetting that I didn't do this. How would addr size fit in here?
| } else { | ||
| // This is an non-nil IP address that was parsed in the else if | ||
| // above. | ||
| na = wire.NetAddressV2FromBytes(time.Now(), services, ip, port) |
There was a problem hiding this comment.
should we not have an err check here rather and make NetAddressV2FromBytes return an error in its default case? cause if we are host here doesnt match any of the above cases nor any of the cases in NetAddressV2FromBytes, then we will return a non-nil NetAddressV2 here with an empty Addr
There was a problem hiding this comment.
If this branch is reached, then ip is either an IPv6 or IPv4 address. The parsing is a little funky because the above branch actually sets ip here. So NetAddressV2FromBytes should hit the ipv4/ipv6 case and return no problem
| @@ -585,7 +592,7 @@ func (p *Peer) ID() int32 { | |||
| // NA returns the peer network address. | |||
| // | |||
| // This function is safe for concurrent access. | |||
| func (p *Peer) NA() *wire.NetAddress { | |||
| func (p *Peer) NA() *wire.NetAddressV2 { | |||
There was a problem hiding this comment.
i think these peer changes need to be in the previous commit else it does not compile.
| if addr.IsTorV3() { | ||
| continue |
There was a problem hiding this comment.
seems we check if the addr is tor v3 both here and in ToLegacy. cant we just return a "NotSupported" error or something from the default case in ToLegacy?
There was a problem hiding this comment.
I guess it's a matter of preference of receiving an error vs receiving a bool or nil
| var protoVersion uint32 | ||
| p.flagsMtx.Lock() | ||
| protoVersion = p.protocolVersion | ||
| p.flagsMtx.Unlock() | ||
|
|
||
| if err := p.writeSendAddrV2Msg(protoVersion); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
out of interest (more a question about the spec, not the impl) why do we need to send a sendaddrV2 message to indicate that we want addrv2s if we have already gating this on negotiated version? ie, cant the peer tell from our version that we want v2 and vice versa?
There was a problem hiding this comment.
Since addrv2 did not come with a version bump (wtxidrelay also uses 70016 - WTXID_RELAY_VERSION in bitcoin core code), it wouldn't be possible to know if your peer supports addrv2 on the version alone (maybe they only support wtxidrelay). This approach is used by bitcoin core now to introduce new feature via messages (during handshake). I think version bumping could have been done, but maybe there's some other reason. In other words, I dont know
There was a problem hiding this comment.
Yeh agree that a version bump should have been done instead, as the current interactive negotiation stuff just serves to make the logic to figure out which features you can use more complicated, vs the way we do things on the LN side re looking at the feature bit intersection.
Commit overview:
wire: netaddressv2 and testsadds (de)serialization routines for BIP-155 addresses. It also allowswire.NetAddressto be converted intowire.NetAddressV2.multi: update addrmgr, server to use NetAddressV2 instead of legacyupdates the address manager to usewire.NetAddressV2internally and does so without a database upgrade. The server is also updated to usewire.NetAddressV2where appropriate.peer+wire: add addrv2 message, protocol negotiationimplements the p2p negotiation of BIP-155 (sendaddr,addrv2) and updates the peer to usewire.NetAddressV2where appropriate. The tests here were modified to not useio.Pipein some cases due to the implementation of the BIP-155 "handshake" not working with a synchronous read/write pipe.Closes #1671 as this pull ignores unknown messages both during version-verack (as does bitcoind) and after the version-verack handshake.
Fixes #1671