< fanquake>
jonasschnelli: Will you be uploading some macOS sigs soonish? I can only see win sigs for 0.18.1 so far.
< jonasschnelli>
fanquake: I will take a look when I'm back in my office (3-4h).
< fanquake>
jonasschnelli: thanks!
< kallewoof>
jnewbery: see my comment on your PR
< fanquake>
promag: re steps to reproduce #16307 (sorry it's taken so long). The best I can give you is just spam load and unload wallet actions from the GUI. "Eventually" it might happen.
< provoostenator>
Fun fact, and something to be aware of when reviewing: if Travis flags the account of a contributor, it won't build their pull request.
< provoostenator>
This doesn't show up as a failure! You still see a green check mark under the PR because of AppVeyor.
< provoostenator>
Also Travis doesn't email you when they flag your account, which is I why I didn't found out for days, see e.g. #16555
< bitcoin-git>
[bitcoin] MarcoFalke closed pull request #16465: test: Test p2sh-witness and bech32 in wallet_import_rescan (master...1907-testAllAddressTypesImport) https://github.com/bitcoin/bitcoin/pull/16465
< bitcoin-git>
[bitcoin] MarcoFalke reopened pull request #16465: test: Test p2sh-witness and bech32 in wallet_import_rescan (master...1907-testAllAddressTypesImport) https://github.com/bitcoin/bitcoin/pull/16465
< jnewbery>
provoostenator: contributors' accounts have been flagged in the past because it looks like they're using travis for mining. An email to support usually gets in unblocked pretty quickly.
< MarcoFalke>
provoostenator: Going to send them an email. I do this every couple of weeks, and I have an email template
< MarcoFalke>
God I can't wait to get rid of travis
< provoostenator>
Thanks, I sent them an email earlier today.
< jnewbery>
kallewoof: thanks. Will look at it this morning. I think behaviour is basically unchanged, except for log spam.
< MarcoFalke>
I guess it makes sense to revert the "Remove redundant checks" commit
< MarcoFalke>
Assuming a wallet with 100k txs, this will fill up the debug log in no time
< dongcarl>
For addrv2, it seems that version messages are also affected... Not sure what the best way to resolve is. If we keep as is, then what do Torv3 senders/receivers set as their `addr_{from,recv}`? If we change to new serialization, old nodes will be confused. It seems that we need a versionv2 as well, that's upgraded to by first sending a versionv1? (kind of messy)
< jonasschnelli>
dongcarl: good point. Maybe wumpus have made some thoughts already on this. Probably something for the thursday meeting.
< dongcarl>
jonasschnelli: Sounds good, I'll bring it up
< wumpus>
dongcarl: I don't think the address in the version message is of any relevance to tor, or other protocols where there's no known 'from' address
< dongcarl>
wumpus: what about `addr_recv `?
< * dongcarl>
is complete aware that he might be misunderstanding this whole situation
< dongcarl>
completely*
< sipa>
i think we rely on the addr in the version message to tell a peer through which address we're connecting to them
< sipa>
to figure out which incoming addresses work
< sipa>
which would still exist with TorV3?
< wumpus>
sipa: oh, you're right
< dongcarl>
Oh, I'm only vaguely aware that this piece of logic exists... Is there documentation or code I can read?
< sipa>
i'm not sure it's particularly useful
< wumpus>
dongcarl: maybe this could be part of the https://github.com/bitcoin/bips/pull/766#issuecomment-517003833 sendaddrv2 message here to notify peers of addrv2 support (which was poroposed as alternative to the protocol version bump the BIP currently documents)
< wumpus>
dongcarl: I think the handling of the VERSION message is the only 'documentation' for this
< sipa>
a sendaddrv2 message would go after a version message though
< * dongcarl>
thinking
< sipa>
so maybe the proposal should be to send a specific dummy address in the version message when v2 is in use
< wumpus>
sipa: yes, so it can extend the version in the version message
< sipa>
and send the actual one inside the sendaddrv2
< wumpus>
yes, that
< dongcarl>
sipa: Yeah that's what I'm thinking
< wumpus>
I don't want to change the version message itself for this
< wumpus>
that's even worse than changing the protocol version :-)
< dongcarl>
wumpus: I'm not 100% sure why we need sendaddrv2 though, why doesn't the protocl VERSION bump work?
< sipa>
dongcarl: because protocol version bumps require monotonically increasing features
< wumpus>
dongcarl: please read the discussion in that PR
< wumpus>
dongcarl: (esp marcofalke's point)
< dongcarl>
wumpus: When I talked to MarcoFalke in person, he said it doesn't matter that much... But I can see the point for feature-monotonic protocol versions
< dongcarl>
A little uncomfortable about adding another protocol message though...
< wumpus>
if we need an extended-version message anyway, then this could have the two purposes at once
< * dongcarl>
thinking
< wumpus>
what's wrong with adidng another message?
< wumpus>
at least unknown messages are simply ignored, so they're pretty much free
< dongcarl>
wumpus: true... perhaps a naive thought but I was thinking maybe there needs to be a generic feature-signaling message for non-monotonic features that don't fit in the protocol version model.
< wumpus>
yes, it's unfortunate that that doesn't exist, but let's please not include that in the scope of this
< dongcarl>
wumpus: Of course not. Could you elaborate a little on how sendaddrv2 would serve dual purpose? I think I'm close to getting it but not quite
< wumpus>
so it would a) signal that the peer sending it can accept addrv2 messages, and b) can contain the wide address that was connected to (e.g. extend what was sent in the version message before it)
< dongcarl>
wumpus: And this would be sent by the connection initiator immediately after its `version` message, right?
< wumpus>
after the version handshake completes
< wumpus>
it's the same as sendheaders
< dongcarl>
wumpus: Okay, and there needs to be a stub netaddr in the version handshake if it's a wide address then?
< wumpus>
yes
< sipa>
yes, because you don't know whether the peer supports v2 yet by that point
< wumpus>
FWIW a lot of alterntative implementations always put localhost in the version message, so that's probably a good dummy
< dongcarl>
wumpus: I was gunna use the SetInternal format on the "addrv2" string
< wumpus>
dongcarl: what do you mean?
< wumpus>
which addrv2 string? what setinternal format?
< dongcarl>
wumpus: you know the special address format we use to encode seed hostnames so that addrman knows where addresses came from?
< wumpus>
that will probably confuse other peers
< wumpus>
e.g. localhost is well known as a 'ignore' value
< wumpus>
that isn't
< dongcarl>
wumpus: roger that
< sipa>
agree
< dongcarl>
easier for non-Core implementers too...
< wumpus>
right, I think so too
< dongcarl>
Okay, I'll try making a PR to the BIP repo, thanks wumpus and sipa for being patient with me
< wumpus>
dongcarl: we should do that, though, we might want to wait with that until we're further along with the implementation, there seem to be many thing no one had realized (like the addresses in the version) until someone tried implementing it :)
< dongcarl>
wumpus: You're right, this entire experience has taught me how important implementation is to spec iteration
< dongcarl>
I'll continue the implementation then, and we'll reconvene when I have something more testable
< dongcarl>
wumpus: One more thing... What will happen in the future if torv42 has 512-bit addresses or something?
< wumpus>
I don't follow the mailing list much
< wumpus>
dongcarl: this scheme is extensible
< wumpus>
it's possible to add new address types to it in another BIP
< wumpus>
old implementations ignore those
< dongcarl>
wumpus: I see, so we'd bump the max size in another BIP
< dongcarl>
wumpus: and the old client would ignore b/c of the max 32-byte restriction in the spec
< sipa>
there isn't a max size, right?
< wumpus>
"Field <code>addr</code> has a variable length, with a maximum of 32 bytes (256 bits). Clients SHOULD reject
< wumpus>
longer addresses.
< sipa>
ah
< wumpus>
I've added a maximum, might want to bump that if we realistically expect protocols with even larger addresses
< wumpus>
or even remove it
< sipa>
this could be an implementation aspect "Implementations MAY ignore address messages of an unknown type, or otherwise impose limits on the maximum size of addr messages for unknown types they relay.
< wumpus>
though, might want to keep some bound for deserialization DoS reasons?
< wumpus>
"Client MAY store and gossip address formats that they do not know about. Further network ID numbers MUST be reserved in a new BIP document."
< wumpus>
they may
< wumpus>
it's not necessary, imo
< dongcarl>
wumpus: yeah, so this way there's no hard cap, but realistically there's a cap based on the max size corresponding to defined network IDs
< sipa>
probably best to recommand not to relay/store unknown types, at which point there is an implied max size anyway
< sipa>
for bandwidth DoS you could have a max size, but it can be fairly large
< wumpus>
the thing is you want to ignore individual unknown addresses even if they're larger than the cap, but not declare the entire addrv2 message they're in invalid
< wumpus>
right
< sipa>
like the limit could be 512 bytes or whatever; it's just so that you can instaban anyone who sends excessive things
< wumpus>
ok, so if an addrv2 message contains any item larger than that, the entire message is invalid
< dongcarl>
just to confirm, we still recommend to ignore individually for addresses that are in between MAX_KNOWN_NETWORK_ID_ADDR_LEN and 512 bytes, correct?
< wumpus>
yes
< dongcarl>
sorry, more accurately, for addresses that have unknown networkID and are below 512 bytes
< wumpus>
they are independent rules a) addresses of unknown type should be ignored, individually, independent of their size b) if the message contains anything larger than 512 bytes the whole message is invalid and anything in it is ignored
< dongcarl>
wumpus: address of unknown type + 513 bytes addr field -> invalidate entire message (rule b), addr of unknown type + 512 bytes addr field -> ignore individually (rule a)
< wumpus>
dongcarl: yes
< wumpus>
addr of known type + wrong-sized addr field -> also ignore individually, I think