< sipa>
use with Using<CompactSizeFormatter<false>>(obj) instead of COMPACTSIZE(obj)
< hebasto>
sipa: re "it's a protection for the case where CompactSize format is used to actually encode a size" in which cases? (`MAX_SIZE = 0x02000000` seems undocumented in the code)
< hebasto>
it was added with alert system in 401926283a200994ecd7df8eae8ced8e0b067c46
< hebasto>
right?
< hebasto>
as alert system has been removed could we just revert back s/MAX_SIZE/INT_MAX/ ?
< sipa>
hebasto: most satoshi commits were aggregates of a bunch of unrelated things
< sipa>
MAX_SIZE is the maximum size of serialized objects, 32 MiB
< hebasto>
I see. Thanks!
< hebasto>
sipa: do you think that `TxRequestTracker::CountCandidates` is really required to be a part of `TxRequestTracker` public interface?
< sipa>
hebasto: it makes testing easier
< sipa>
but apart from that, it's unneeded - for now at least
< sipa>
it could be used as part of an overload check later
< bitcoin-git>
bitcoin/master faf2999 MarcoFalke: cirrus: Use kvm to avoid spurious CI failures in the default virtualizatio...
< bitcoin-git>
bitcoin/master 380705e MarcoFalke: Merge #20106: cirrus: Use kvm to avoid spurious CI failures in the default...
< bitcoin-git>
[bitcoin] MarcoFalke merged pull request #20106: cirrus: Use kvm to avoid spurious CI failures in the default virtualization cluster (master...2010-ciOtherVirt) https://github.com/bitcoin/bitcoin/pull/20106
< bitcoin-git>
[bitcoin] MarcoFalke closed pull request #20103: test: Enable mocktime RPC for all test chains (master...2010-testMockAllChains) https://github.com/bitcoin/bitcoin/pull/20103
< vasild>
hi
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #20112: test: Speed up wallet_resendwallettransactions with mockscheduler RPC (master...2010-testFasterMock) https://github.com/bitcoin/bitcoin/pull/20112
< sipa>
vasild: morning
< vasild>
evening :)
< vasild>
(for you)
< vasild>
is it on purpose that WriteCompactSize() can write big numbers, but ReadCompactSize() cannot read them?
< sipa>
vasild: i think in the compactsize-is-only-for-sizes setting we used to have, the issue just didn't occur
< sipa>
as it's not possible to have such a big object to begin with
< vasild>
yeah, so writecompactsize was never called with big numbers
< sipa>
indeed
< sipa>
(and the usage in bip152 is restricted to 16-bit numbers, which obviously won't have any problems)
< vasild>
I am just curous - any idea what are those high service flags 0x800043d and 0x800000d that cannot be a result of bitwise-or-ing NODE_* flags?
< vasild>
of course any peer is free to claim whatever they want in their service flags, including 0xfffff...
< sipa>
some high bits are used for private extensions
< sipa>
not sure what this is
< vasild>
ok
< hebasto>
sipa: good idea to rename CompactSize
< sipa>
vasild: what software/user agent reports these?
< vasild>
I did not record that
< vasild>
just added printf() in CAddress::unserialize to see what is coming in from the network
< vasild>
now I did -addnode=62.107.200.30:8333 to see the user agent
< sipa>
they're uninteresting
< sipa>
my dns seed crawler sees them too
< sipa>
but they're from a variaety of innocently-loking user agents
< vasild>
but now getnetworkinfo shows
< vasild>
"addr": "62.107.200.30:8333",
< vasild>
"services": "000000000000040d",
< vasild>
:-O
< sipa>
hmm
< vasild>
yeah, and innocent "subver": "/Satoshi:0.20.1/",
< sipa>
are they being rumoured with this bit set... but then not reported in the version message?
< hebasto>
could this hurt the network?
< vasild>
sipa: must be that or something else I am missing
< vasild>
shutdown, restart with -addnode=178.128.96.96:8333
< vasild>
and getpeerinfo: "addr": "178.128.96.96:8333",
< vasild>
"services": "000000000000000d",
< vasild>
we don't do any "filter away any unknown service bits before displaying in getpeerinfo, right?
< sipa>
vasild: i don't think so
< vasild>
just for reference, here is my dummy printf() in CAddress unserialize: https://bpa.st/JYYA
< sipa>
looks right
< vasild>
ok, assuming they get gossiped with the flags but are later not present in the version message when we actually connect
< sipa>
perhaps some broken software is relaying them with that bit set
< vasild>
wumpus: just to confirm - yes, 0.20 would start on a 0.21-modified-peers.dat as if that file was not present, I also think it is ok. Except the obscure/scary deserialize error messages
< sipa>
perhaps we can give a nicer error message in the future for upgrades
< sipa>
but for old software it's too late
< vasild>
given I will be modifying #19954 I will add one line at the start of unserialize to check if the version is too high
< vasild>
and print something like "detected file format of peers.dat from the future: 4, I can only understand up to 3, starting operation as if peers.dat is empty"
< hebasto>
sipa: if their software is broken should we discourage those peers?
< vasild>
maybe a honest/legit/unbroken peer is just relaying some shi^H^Hstuff that was given to him by a broken/malicious guy
< vasild>
we don't want to flag the legit peer as misbehaving
< hebasto>
right
< vasild>
or also those high service bits could have some purpose, I guess, it is not certain that they are due to broken or malicious software
< * hebasto>
"hidden service" :D
< vasild>
did somebody develop a decentralized instant message chat application using bitcoin p2p network, sneaking the data via the service bits?
< MarcoFalke>
it would be good to log the peer that sent the high bits
< sipa>
MarcoFalke: unfortunately, they're most likely not the ones who introduced it
< vasild>
what happens if a node receives via gossip the same address:port but with different service bits?
< MarcoFalke>
does addrman correct the service bits when a version msg is received?
< vasild>
that too!
< sipa>
they get OR'ed together
< sipa>
but i hope that on an actual connection, it's overwritten
< vasild>
they get OR'ed :( so a node cannot correct his by re-advertising it regularly. Once somebody adds some high bits to his addr:port, it is over.
< sipa>
we should fix that
< sipa>
whatever we receive from the node itself should be written exactly into addrman
< vasild>
change to "newer one overwrites old one"?
< sipa>
i think OR'ing makes sense for rumoured addr messages
< vasild>
I agree that if we connect directly then whatever the peer says is to overwrite the other stuff
< sipa>
but if we receive information from the IP itself (on an outgoing connection), it should overwrite
< jonatack>
catching up with the discussion... yes, the issue I reported is the DB:deserialise error messages and peers.dat being recreated; bitcoind operates fine otherwise. We can't fix the older software, but we can communicate the situation clearly to users (and hopefully fix it henceforth).
< wumpus>
sipa: it's more of a warning message anyway isn't it, people that don't regaularly check the log won't even notice it
< wumpus>
vasild: thanks for confirming, I think there's no problem in that case at all
< wumpus>
if you say "error" I was expecting some kind of emergency shutdown, that *would* be scary, it simply saying that it's unable to load the file and start from scratch isn't scary as such
< vasild>
y
< jonatack>
well, anyone who greps the past log for errors will see
< jonatack>
ERROR: DeserializeDB: Deserialize or I/O error - ReadCompactSize(): size too large: iostream error
< jonatack>
Invalid or missing peers.dat; recreating
< wumpus>
yes
< vasild>
I though jonatack's disk has developed bad sectors :-D
< jonatack>
that allcaps screaming ERROR is a bit worrisome :D
< vasild>
thought
< wumpus>
I agree it could have been avoided with a version marker
< wumpus>
jonatack: that's nothing, those errors used to appear for malformed incoming *network packets*
< wumpus>
ERRORS i mean
< wumpus>
debug.log used to be one big file full of screaming :)
< jonatack>
vasild: restarting on, say, 0.18.1, I see "ERROR: DeserializeDB: Deserialize or I/O error - CAutoFile::read: end of file: iostream error" and "Invalid or missing peers.dat; recreating", as we mention in the new release notes, but not on 19954 after `cp onion_private_key onion_v3_private_key`
< jonatack>
hebasto: saw the same yesterday, i gave up trying to see the discussion :/
< vasild>
gh censorship!
< michaelfolkson>
I don't see how Core is still managed on GitHub in 10 years personally. But will be a massive headache whenever it does move. Ideally done when things are quiet (not with a potential soft fork on the horizon)
< jonatack>
ISTM that GitHub is now starting to hide ACKs and re-ACKs as "one similar comment"
< wumpus>
I'm not sure moving is that much of a headache, many projects have done this, it's just that the only thing after github that makes sense is self-hosting something like gitlab or gitea, which has quite a lot issues of its own, e.g. who hosts the instance
< wumpus>
I have a weird issue on my freebsd node: it kept making outgoing connections at a rate of ~2 per second, the whole log is full of it, and it wasn't accepting any new blocks. I hope it's not a problem with the TorV3 PR.
< wumpus>
also, it segfaulted on shutdown (I can't reproduce this, unfortunately, so don't have a traceback)
< jonatack>
wumpus: huh. i have not been seeing that issue with 19954 (on debian)
< wumpus>
it's not happening on my other TorV3 node though so there's no clear correlation
< wumpus>
jonatack: right, it might be a local issue for that node
< wumpus>
currently building plain master
< luke-jr>
wumpus: I'm not sure self-hosting is really an improvement over GitHub, aside from the platform presumably being free software
< luke-jr>
the goal is probably a decentralised system, but that doesn't exist yet afaik
< vasild>
there were some attempts to make decentralized alternative of github
< wumpus>
luke-jr: it's an improvement in that we'd have direct control over the software, so it can't say, start hiding consecutive ACKs behind our back
< vasild>
wumpus: I have this sysctl on fbsd: kern.corefile=/coredumps/%U.%N.%P.core
< wumpus>
vasild: good idea
< vasild>
and /coredumps has 1777 perms so that anybody can plant files in it
< luke-jr>
wumpus: whoever is hosting can, whether that's GitHub or ⁇?
< wumpus>
luke-jr: sure, but if it's one of us they're likely going to be more recentive to these kind of complaints than a big corporation
< vasild>
ulimit -c is "unlimited" but I don't think I configured that myself, maybe it is the default
< wumpus>
same here, it even said in dmesg that it had dumped core but couldn't find the core file
< sipa>
it dumped Core
< luke-jr>
they tend to be big files
< vasild>
does bitcoind change the working directory at startup? to ~/.bitcoin? or to the root /?
< wumpus>
sipa: heh
< wumpus>
vasild: no, it doesn't
< wumpus>
thinking of it, probably daemonize() does though
< vasild>
so the core should probably in the current directory where you started it
< luke-jr>
I know Linux is capable of dumping core into a gdb process directly
< wumpus>
yes, daemon() changes the working directory to /, it has no write access there, so I guess it just lost it
< wumpus>
I've set vasild's sysctl parameter now so next time it should go to /coredumps/
< vasild>
remember to rm /coredumps/* periodically because it grows and grows over time ;)
< vasild>
wumpus: but if dmesg says something like "pid 94776 (conftest), jid 0, uid 0: exited on signal 11 (core dumped)
< vasild>
with "(core dumped)" at the end then it must be somewhere
< wumpus>
vasild: I couldn't find it at least, but, I 'make clean'ed so deleted the binary by now so I guess there's no point even if I still found it
< luke-jr>
hmm
< luke-jr>
does C undefined behaviour extend to compile time?
< luke-jr>
can the compiler explode the system when you try to compile it?
< sipa>
luke-jr: no
< sipa>
(afaik)
< wumpus>
don't give compiler authors any ideas
< luke-jr>
nuts, could have submitted a patch to GCC to erase everything when it encounters mem*
< luke-jr>
XD
< luke-jr>
wumpus: hey, at least I didn't suggest telemetrics on UB?
< wumpus>
the most disappointing thing about the whole C UB sitution is how little, historically, C compilers have attempted to detect and reject it, I mean if mem* is only reserved for system functions, fair enough, but make it an error or warning.
< wumpus>
but no they really like their random explosions :)
< sipa>
the reason i suspect is that they don't want programs to misbehave if memslartbartfast suddenly becomes a new standard library function in C2043, and your code is already using it
< luke-jr>
they probably would prefer a C22 compiler is C99-compatible too
< luke-jr>
by making mem* UB, a C22 memfoo doesn't break C99 code
< luke-jr>
(same thing, but from another angle)
< sipa>
i guess a better alternative would be to have a way for the STL to declare "reserved but unused" name patterns to the compiler, so it can error if you use a reserved name, rather than needing to resort to the super-heavy "UB" hammer
< wumpus>
looks like I have the same problem again after merging 19954: my node keeps making outgoing connections, but never gets any blocks
< sipa>
hmm
< sipa>
this looks familiar
< wumpus>
somehow it's dropping all outgoing connections immediately, it stays at 1-3
< sipa>
Murch: do you remember someone on bitcoin SE complaining about similar behavior?
< wumpus>
it does succesfully accept incoming connections but that doesn't help
< luke-jr>
wumpus: to clarify, you mean without 19954 it was okay?
< wumpus>
luke-jr: yes, without that it seemed to be okay
< wumpus>
it did receive a few blocks
< wumpus>
but no crash at shutdown this time
< wumpus>
but it does look like 19954 is the problem, will try again without that
< sipa>
oh, ok
< sipa>
in that case what i remember seeing must have been unrelated
< wumpus>
probably, unless this triggers a similar condition
< wumpus>
I did already try deleting peers.dat
< achow101>
anyone ever see "corrupted size vs. prev_size in fastbins" on stderr before? it's showing up on a change to sqlite wallet that I'm working on
< sipa>
achow101: i wouldn't know what prev_size or fastbins are
< wumpus>
googling it, it's a glibc heap corruption error
< achow101>
it seems to be coming out of libc
< wumpus>
but to answer your questino, no, I've never seen it
< achow101>
great..
< luke-jr>
valgrind it
< achow101>
good idea
< roconnor>
very early on, the glasgow haskell compiler had an infamous bug where it would delete the source file if you tried to compile it and it had a type error.
< luke-jr>
XD
< sipa>
roconnor: type errors are *really* frowned upon
< achow101>
ah, I had a double free
< sipa>
achow101: see youtube link above
< achow101>
heh
< wumpus>
gah, the peer keeps disconnectiong outgoing connections without logging the reason at all (even with debug=net)
< meshcollider>
2:09 AM <achow101> #proposedwalletmeetingtopic: wallet.dat vs wallet.sqlite
< meshcollider>
#topic wallet.dat vs wallet.sqlite
< achow101>
for sqlite wallets, there's been an ongoing question of whether the sqlite wallet files should be named wallet.dat or wallet.sqlite
< hebasto>
what are pros and cons of each approach?
< achow101>
the PR currently implements wallet.dat. ryanofsky has been arguing for wallet.sqlite in his review comments
< achow101>
i wanted to hear what everyone else thinks
< Murch>
hello
< hebasto>
I've just started to review that pr, so have no opinion
< achow101>
for wallet.dat, the arguments are to maintain backwards compatibility with external documentation and tooling, as well as not causing a problem with a specific downgrade scenario
< meshcollider>
Yeah calling it wallet.dat has the advantage that automatic backup scripts, etc. will continue working fine, and also that users are already conditioned to protecting wallet.dat files
< achow101>
for wallet.sqlite, it's a clearer naming convention, follows sqlite naming convention, and can't be confused with bdb
< jonatack>
hm, good arguments for both
< achow101>
wallet.sqlite also avoids a different set of compatibility prolems
< sipa>
yeah, i'm very much in the middle
< sipa>
some of the naming conventions and expectations around them were already broken when we moved to per-wallet directories
< sipa>
and i don't recall that causing many issues for users
< sipa>
though, the specific "wallet.dat must be protected with your life" filename convention remained
< luke-jr>
meshcollider: it is already wrong and risks corruption to copy wallet.dat directly
< fjahr>
hi
< hebasto>
if users adopted per-wallet dir, we could expect such adoption for .sqlite
< sipa>
luke-jr: it doesn't if you do it while bitcoind is shut down
< meshcollider>
luke-jr: that doesn't stop users doing it
< luke-jr>
meshcollider: breaking such scripts would be an advantage to renaming
< sipa>
luke-jr: i couldn't disagree more with that
< luke-jr>
a possible issue is restoring though
< achow101>
luke-jr: but breaking such scripts would probably result in backups not being made, which is dangerous
< luke-jr>
achow101: most likely would result in errors instead of possibly-corrupt backups
< achow101>
I would be surprised if said scripts failed in a way that was obvious to the person running it
< luke-jr>
O.o
< sipa>
luke-jr: i agree that we should discourage bad practice, but (a) not by making decisions that can actually cause people to lose money and (b) i disagree this is unsupported - it's only supported when bitcoind is not running though
< meshcollider>
And there are also other tools I imagine, not just backup scripts, which look for wallet.dat by default
< luke-jr>
if a cronjob fails, typically you get an email
< luke-jr>
sipa: at least some versions would reuslt in corruption even if bitcoind exited cleanly
< achow101>
luke-jr: fwiw I have a system backup cronjob and I don't know when/if it fails until I check the logs, and that happens maybe once every 6 months
< sipa>
how so?
< luke-jr>
sipa: we used to not flush/close the db
< luke-jr>
achow101: you should fix that :p
< sipa>
luke-jr: i believe that was very briefly the case, in ancient times
< achow101>
luke-jr: right, but that's an example of a backup script failing and the user not knowing
< luke-jr>
I suppose people doing backups wrong, are also likely to do error notifications wrong
< sipa>
people will do lots of things wrong
< sipa>
doesn't mean we shouldn't do a best effort to avoid them losing money
< luke-jr>
but wallet.dat are currently in a dedicated directory
< luke-jr>
there's no need for that for sqlite, right?
< sipa>
that's a good question
< achow101>
luke-jr: yes, but I think it would be more confusing to users if we stopped doing that
< sipa>
hmm
< jonatack>
modulo the risk of users losing money if renamed, a risk i don't feel competent to evaluate, i tend to agree with ryanofsky's arguments
< sipa>
to me, that'd be one of the advantages of sqlite... not needing a directory for every wallet anymore
< luke-jr>
btw, even if it's wallet.sqlite, it's not like we're renaming without the user knowing
< luke-jr>
wouldn't you expect anyone setting up a backup script to check that it works when they create the wallet, at least once? :P
< achow101>
luke-jr: not necessarily. they'd need to read the release notes, and who the hell does that?
< luke-jr>
btw, I wonder if creating a wallet is really the right response to not finding one named :P
< roconnor>
In a different program I complained where running an obviously read-only operation would create a new file if the required file didn't exist.
< achow101>
luke-jr: it definitely isn't and we should remove that behavior