< bitcoin-git> [bitcoin] leozz37 opened pull request #20116: Implemented GitHub Actions workflow for MacOS and Ubuntu (master...github-actions) https://github.com/bitcoin/bitcoin/pull/20116
< yanmaani> I'm getting weird errors while building Bitcoin Core, but everything seems to work fine
< yanmaani> > /usr/bin/ld: error: leveldb/libleveldb.a(libleveldb_a-crc32c.o): <corrupt x86 feature size: 0x8>
< yanmaani> for what seems to be every .o file
< 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
< Murch> Yes, let's all learn Esperanto or Klingon
< Murch> Although, this had nothing to do with homophones or irregular pronunciation :p
< sipa> put and put are arguably homophones
< Murch> more importantly for my communication mishap, they're homonyms
< fanquake> Murch: I wasn't trying to correct your spelling hah, I meant I'd just tagged you in #20040.
< gribble> https://github.com/bitcoin/bitcoin/issues/20040 | wallet: Refactor OutputGroups to handle fees and spending eligibility on grouping by achow101 · Pull Request #20040 · bitcoin/bitcoin · GitHub
< Murch> fanquake: I thank ye kindly!
< achow101> moar refactor
< fanquake> I've passed on more feedback to GitHub, now that we're seeing even more comment related issues: https://0bin.net/paste/9HgV-Dyl#sZlLldWLuSFQGiT1-yYS5ikMyCIIkRLd5lK3fuAD/ql & https://imgur.com/a/mUVsDSX.
< tryphe> sipa, such a great piece
< tryphe> or how about white night, wight knight
< tryphe> or white knight and wight night :D
< 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> vasild: phew, we can ignore the issue, https://github.com/bitcoin/bitcoin/pull/19954#issuecomment-705504702
< 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!"
< gribble> https://github.com/bitcoin/bitcoin/issues/18750 | build: optionally skip external warnings by vasild · Pull Request #18750 · bitcoin/bitcoin · GitHub
< 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
< jonatack> though it was documented as uint32_t in https://en.bitcoin.it/wiki/Protocol_documentation#Inventory_Vectors
< 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
< jonatack> luke-jr: right (https://en.cppreference.com/w/cpp/language/types), recent change was signedness
< luke-jr> not sure it matters :P