< yanmaani>
didn't find anything about it on the Internet. Is this a known issue?
< bitcoin-git>
[bitcoin] fanquake closed pull request #20116: Implemented GitHub Actions workflow for MacOS and Ubuntu (master...github-actions) https://github.com/bitcoin/bitcoin/pull/20116
< sipa>
vasild: i was wrong; when making an outbound connection, we overwrite the addrman services with the peer's reported ones
< sipa>
see CAddrMan::SetServices
< Murch>
jonatack: okay, put it on my list
< fanquake>
Murch: I just put something on your list
< Murch>
sorry, I meant that I had put it on my list. I realize now that the conjugations of this particular irregular word make my brevity particularly misleading
< wumpus>
sipa: no, not really since last night, it seems to be a curious case of miscompilation, I still have a broken binary but can't reproduce it anymore when building even exactly the same commit. Things have been working since.
< wumpus>
my only guess is a hardware problem or a weird bug with FreeBSD's clang 8-derived compiler. The binaries are sufficiently different too, it's not like there's a few bytes different so it's easy to compare.
< sipa>
okay
< sipa>
i couldn't reproduce it either
< sipa>
but i don't know the exact circumstancs
< sipa>
if you can't even reproduce it with the same commit (and same peers.dat?), i guess we'll need to assume it was a very local problem
< wumpus>
in any csae, if anyone still wants to figure it out, I two binaries built from the same commit, one with the issue one without
< sipa>
the binaries differ?
< wumpus>
yes, very significantly
< wumpus>
stripped size is the same but everything is in a different place
< sipa>
different compiler/compileflags?
< wumpus>
no, not intentially at least
< wumpus>
might have been some cruft in the build dir, who knows... the curious thing is that it happened with 19954 and not without though. I still can't explain that.
< wumpus>
but I don't think it's anything to worry about for that PR
< wumpus>
(no, peers.dat nor anything in the data directory is at fault, it only depends on what binary is run)
< sipa>
ok
< wumpus>
I guess it will always remain a mystery, running that particular binary still has the same issue: every outgoing peer is disconnected quickly (usually after sending feefilter), no reason logged in debug.log
< wumpus>
(correction: *always after "sending feefilter (8 bytes) peer=X" there's "disconnecting peer=X", sometimes with "Cleared nodestate for peer=X" in between*)
< sipa>
anything interesting if you run in valgrind?
< wumpus>
havne't tried that yet
< wumpus>
it could be thinking that every outgong connection is a feeler connection, it's the only fDisconnect I could find without associated NET logging so quickly
< wumpus>
I wonder what level of debug information FreeBSSD toolchain adds by default
< wumpus>
given that there's 95MB of size difference between the stripped binary and the plain one, quite a lot would be my guess
< wumpus>
could try setting some breakpoints
< wumpus>
sipa: ... okay ... after finally figuring out how to set a hardware memory breakpoint on an atomic value (to see when fDisconnect changes), I discovered something shocking: two parts of the program have different CNode layouts
< wumpus>
sipa: there's an off-by-one where pfrom.fSuccessfullyConnected = true; happens to set pfrom.fDisconnected = true ...
< wumpus>
sipa: so it's clear, a miscompile! I think a stale .o file somehow as linked in, I have --disable-dependency-tracing as it wouldn't build on FreeBSD otherwise
< wumpus>
anyhow happy I got to the bottom of this, a "silent link issue" must be one of the most evil things I've ever debugged
< hebasto>
wumpus: great to know it
< vasild>
sipa: right, so we overwrite the service flags when we outbound-connect to a peer from the received version message, and via gossip we | the flags together if we already have an entry for that addr:port
< vasild>
wumpus: ccache?
< wumpus>
vasild: no, lack of dependency tracking inside automake
< vasild>
this is what I wrote to hebasto just a few days ago when discussing #18750: "long time ago I ran into some obscure compilation issues and after long debugging I figured out that it is due to ccache and I have never been using it after that!"
< vasild>
wumpus: "I have --disable-dependency-tracing as it wouldn't build on FreeBSD otherwise" -- hmm, I don't use that option and it compiles just fine
< vasild>
wen cmake?
< vasild>
:-D
< fanquake>
hopefully no time soon
< fanquake>
We’ve got lots more important things to deal with than swapping build systems, and god forbid we have to maintain two at once.
< wumpus>
vasild: you are probably following the new, better build instructions for freebsd that tack on MAKE=gmake instead
< vasild>
wumpus: I run ./autogen.sh && ./configure && gmake
< wumpus>
all build systems have their own set of warts using cmake would just switch one for the other
< wumpus>
vasild: that doesn't work for me
< wumpus>
(and for many other people it doesn't either which is why that is in the build instructions)
< wumpus>
might be a automake/autoconf version thing but in any case I'm not very interested in debugging that now :-)
< vasild>
I saw some mentions of build failures and --disable-dependency-tracing but never bothered to study that, especially that it works for me :)
< vasild>
I mean build works for me without --disable-dependency-tracing
< wumpus>
disabling dependency tracking will result in madness and insanity don't do it, override MAKE intead
< ariard>
vasild: I'm still not understanding how a BIP155 node is supposed to discover the list of network IDs supported by its BIP155-peers ?
< ariard>
for now the table is hardcoded but I assume it will be extended in the future
< hebasto>
sipa: could protecting `m_txrequest` with its own mutex rather with `cs_main` be safe and provide more concurrency?
< sipa>
hebasto: no, because all call sites already hold cs_main
< hebasto>
sipa: ok, thanks
< sipa>
it's also pointless, as net_processing is inherently single-threaded (there is only one message handler thread)
< sipa>
i think net_processing should move to its own locks, instead of cs_main, but that's a larger scope thing, and doing that for just m_txrequest doesn't provide benefits until the rest of the file can make use of it
< hebasto>
then why protecting `m_txrequest` at all?
< sipa>
?
< hebasto>
if the only thread has access
< sipa>
because we don't want to introduce bugs
< sipa>
if the code is perfect, all thread annotations/checks are wasted effort
< sipa>
we could also delete all tests ;)
< hebasto>
no, please :)
< sipa>
it isn't the case that only one thread has access - it's that (i assume!) in the current code, only one thread actually accesses it
< hebasto>
re "i think net_processing should move to its own locks, instead of cs_main" agree
< stevenroose>
is there any protocol rule limiting future env type identifiers to 32 bytes?
< stevenroose>
or can it in principle be any length?
< sipa>
"env type identifiers" ?
< stevenroose>
inv*
< stevenroose>
from the inventory messages
< stevenroose>
txid, wtixd, block hash, ...
< stevenroose>
sipa: ^
< stevenroose>
all current ones are some variant of sha256 hash, but I'm not sure if that's a hard rule
< sipa>
no, they're just 32-bit numbers
< stevenroose>
I mean CInv is currently hash256, so Core would refuse any future inv variants that are > 32 bytes. perhaps that's enough to make sure it won't every be any other length
< sipa>
the current protocol defines a CInv as 32-bit type + 256-bit value
< stevenroose>
key thanks
< sipa>
there isn't even a way to convey anything else
< sipa>
it's not like the "CInv value length" is sent anywhere
< sipa>
a negotiated protocol change could modify that of course
< stevenroose>
ah good point there is no length indicator, hadn't thought about that
< jonatack>
stevenroose: until very recently, CInv type was implemented as an int and was changed to uint32_t in #19818
< gribble>
https://github.com/bitcoin/bitcoin/issues/19818 | p2p: change `CInv::type` from `int` to `uint32_t`, fix UBSan warning by jonatack · Pull Request #19818 · bitcoin/bitcoin · GitHub
< stevenroose>
jonatack: awesome :) yeah in rust-bitcoin it's been a u32 always AFAIK, but I was confused, my question didn't make much sense because as Pieter pointed out, the inv message can only have 32-byte hashes as there is no way to convey items of other lengths
< jonatack>
(by MagicalTux and harding, respectively)
< luke-jr>
jonatack: on all supported platforms, int is 32-bit