< tryphe>
sipa, i'm pretty sure windows marks applications as "hung" after a certain number of seconds if the window handle stops responding to an optimistic message, and closes any applications that are "hung"
< tryphe>
there's a registry value called HungAppTimeout that you can change to modify the timeout, though. not sure if that's useful though.
< tryphe>
seems like the right thing to do would be to figure out why it doesn't respond to that message, but it might be a bit hairy trying to debug that
< sipa>
tryphe: yes, but that doesn't sound like what's going on here
< tryphe>
could be, but i have some machines where that behavior happens to other applications, not just bitcoin (i don't run bitcoin on windows though so can't say)
< vasild>
I was thinking that it is possible to change the py testing framework to always announce support for addrv2 which would flip all tests to use addrv2 but then addr will be untested... hmm
< wumpus>
tryphe: oh no, if so it's just a new symptom of the old 'we hang the UI event loop on everything' problem
< wumpus>
in any case I don't remember the windows shutdown detection code changing recently
< wumpus>
but it would be good if anyone that runs windows could check this
< jonatack>
vasild: indeed, i wondered how much extra effort it would be to test both and started doing it, then stopped before the rabbit hole "nah...i'll ask the question" ;)
< vasild>
git grep msg_addr ./test/functional/
< wumpus>
not seeing anything that could break the shutdown monitor in the git log for winshutdownmonitor.cpp, the last change of any significance was removing OpenSSL seeding in 2019, which jus removes an if() statement, before that it effectively wasn't teouched since the file was first introduced
< fanquake>
wumpus: I did try this morning and from what I remember I didn't see a shutdown window
< fanquake>
Will test again
< vasild>
jonatack: p2p_addr_relay.py is the only test that uses msg_addr!?
< wumpus>
fanquake: okay, worrying then :/
< jonatack>
vasild: seems so
< vasild>
jonatack: I assumed there were many others that would keep testing msg_addr when p2p_addr_relay.py is converted to use msg_addrv2, but if that test is the only one testing msg_addr, then we better not stop it
< jonatack>
yes, initially it didn't seem to be too much extra effort to test both
< wumpus>
the only thing I can think of, besides Windows deprecating this way of detecting shutdown, is that it might be one of the Qt updates breaking how native event filters work?
< wumpus>
I honestly don't know though I'd expect much more complaints if this really doesn't work anymore
< vasild>
to test that compilation works on more platforms than CI?
< vasild>
btw, guix_build.log for master is 35M while for master+PR 3.4MB
< fanquake>
wumpus: I tried again, and didn't see a shutdown dialog, but that's also might be because Windows threw up another shutting down overlay, which might have hid it.
< wumpus>
fanquake: maybe a test like would be useful: add a sleep during shutdown to make sure it's slow, then shut down windows, afterwards check the log if it got to the end or was interrupted
< wumpus>
also maybe check for the message (if we log any) that the shutdown request came in
< wumpus>
after all, the issue here would be not so much not seeing the shutdown window but that it shuts down unclean
< fanquake>
wumpus: I'll take a look at it tonight
< wumpus>
thanks!
< wumpus>
(looking at the code: no we don't log any specific message if WM_QUERYENDSESSION comes in, nor on registering the shutdown handler itself, though it does log if it fails for some erason)
< wumpus>
it might make sense to add those things to make troubleshooting easier
< kallewoof>
i get a bunch of errors about inline asm when i try -fsanitize=address on a mac. is that normal?
< bitcoin-git>
bitcoin/master 6fccad7 Jon Atack: signet: do not log signet startup messages for other chains
< bitcoin-git>
bitcoin/master 9fc2f01 MarcoFalke: Merge #20048: chainparams: do not log signet startup messages for other ch...
< bitcoin-git>
[bitcoin] MarcoFalke merged pull request #20048: chainparams: do not log signet startup messages for other chains (master...signet-startup-logging-bugfix) https://github.com/bitcoin/bitcoin/pull/20048
< MarcoFalke>
vasild: The builds are just to confirm our guix/gitian build doesn't break. I ask DrahtBot to do those on "risky" build system changes
< vasild>
MarcoFalke: ok, did it break? (it is not obvious to me from the logs, but I think not)
< MarcoFalke>
no, it didn't break ;)
< vasild>
do you think that it makes sense to do one test run with the new option enabled? because by default it is disabled which means no change in behavior
< aj>
sipa: hmm. every single binary in src/test/fuzz is ~185MB for me (following the instructions in doc/fuzzzing.md). 117MB stripped, by the looks. total of 18G for src/test/fuzz, 22G for src
< wumpus>
aj: that's large even for a static build
< aj>
wumpus: hmm, bitcoind is 83M unstripped, 16M stripped for me
< promag>
please remove mine from hp, it doesn't help getting more reviews
< wumpus>
ok
< wumpus>
yes, anything concerning libevent initialization/shutdown is notoriously hard to review
< promag>
basically it will wait for you ^^
< wumpus>
well the PR seemed really tricky to me... I think what we need is a clear test, and then to test with different versions of libevent
< wumpus>
I can't judge just from the code change wether it's better or not
< promag>
ok, then I guess I need to make it clear and improve testing!
< wumpus>
it's kind of awkward we've had a history of PRs like that, and it never really fixed the problem, we'll really want to make sure we get it right this time or it doesn't make sense to make another change there
< aj>
i thought before the branch was still plausible; also prioritising 19988, but i think it's just about there
< promag>
no backport after branch off right?
< luke-jr>
promag: softforks should always be backported to anything supported
< luke-jr>
not necessarily until activation is set tho
< wumpus>
yes, softforks are always backported
< wumpus>
but it would be good to get that in before the branch-off if it's ready anyway
< jnewbery>
it currently has two ACKs (instagibbs and benthecarmen). Feels like now would be the time to review if anyone was waiting for the right moment!
< ariard>
sounds good to prioritize 19988, I had the same PR order
< jonatack>
agree. i was thinking the same as aj and sipa WRT these two PRs, 19953 and 19988
< jonatack>
(along with the tor v3 support)
< sipa>
it seems silly to say "merge right after branch", if it's ready to merge at that point, it's also ready to merge before
< achow101>
i'll move it to the top of my review queue
< wumpus>
sipa: right, 'merge after branch' is for features that shouldn't be backported
< sipa>
or for very invasive refactors perhaps
< wumpus>
yes
< sipa>
of course, if it isn't ready before, it isn't, and that should be that
< jnewbery>
sipa: I meant "if it's not ready to merge at branch, then it should be a priority to get it reviewed/merged as soon as possible", not "don't merge it before the branch even if it's ready"
< sipa>
jnewbery: right, that makes sense
< sipa>
in any case, the outstanding things i have for 19953 at this point are a few review comments (which will just result in added code comments for clarification), and documenting the process of creating the JSON unit test data
< ariard>
I still want to review in-depth the test coverage, but that said 14 days is largely enough
< wumpus>
which is important but doesn't hold up the code merge
< sipa>
and obviously addressing any review comments that may still arise
< jnewbery>
I guess maybe the message should be: feature freeze is in two weeks, think about what your priorities are. Mine are 19988 and 19953.
< sipa>
i'm committed to quickly addressing comments in both of those
< sipa>
my other priority is torv3
< luke-jr>
IMO #11082 is important to get into 0.21, but clearly I can't do it alone :/
< gribble>
https://github.com/bitcoin/bitcoin/issues/11082 | Add new bitcoin_rw.conf file that is used for settings modified by this software itself by luke-jr · Pull Request #11082 · bitcoin/bitcoin · GitHub
< aj>
wumpus: oh, i guess #19937 for high-pri might be nice
< sipa>
luke-jr: i haven't seen any argument as for what it adds over settings.json, so barring a wide agreement to revert that (and there doesn't seem to be), it seems hard to get people to find it important
< luke-jr>
sipa: it potentially reduces us to 1 config format, whereas settings.json bumped us to 3 or 4
< wumpus>
aj: added
< wumpus>
sipa: I agree, that's why I haven't really looked at it either
< wumpus>
I don'twant yet another config file, initialization and setting priority is complex enough as it is
< sipa>
luke-jr: but settings.json is merged; you'll need a much stronger case if your goal is reverting that and merging bitcoin_rw instead
< luke-jr>
wumpus: that's the point behind rwconf - it reduces
< sipa>
as having both seems the worst of both worlds
< luke-jr>
sipa: that's why it's important to do before 0.21
< luke-jr>
once 0.21 is released, we're stuck with settings.json
< sipa>
luke-jr: but your goal isn't just merging bitcoin_rw; it's replacing a feature that was already merged
< sipa>
i have no opinion either way, but one is merged, and the other isn't
< wumpus>
it's an internal settings file, people are not supposed to edit it manually, might be better if it's in json format instead
< wumpus>
could have been some obscure binary format as well but as least this is readible if you want to...
< jnewbery>
luke-jr: you failed to convince people that bitcoin_rw was the right approach in the PRs. I don't see what simply repeating your position here will achieve
< wumpus>
any other topics?
< achow101>
at what point do we switch to the milestone for hi prio?
< wumpus>
achow101: well not yet as I've not really looked at what is at the milestone yet
< wumpus>
but yes good point
< wumpus>
we could do that starting from next week, after sorting things out, I think we need to postpone things like "sqlite wallet storage" to 0.22 https://github.com/bitcoin/bitcoin/milestone/45
< achow101>
:(
< wumpus>
as well as the guix build
< wumpus>
yes, :(
< sipa>
i'm sorry i haven't had the time to dive into that... i'd really like to see sqlite happen
< luke-jr>
"the guix build"?
< wumpus>
but that sounds scary to do last minute at least to me, seems like something that needs to brew in the master branch for a while
< * dongcarl>
awakens
< wumpus>
luke-jr: yes, 0.21 will be another release built with gitian it seems?
< luke-jr>
wumpus: hopefully gitian won't be abandoned any time soon
< luke-jr>
guix is an improvement over Ubuntu, but not over gitian
< achow101>
the main thing was that I had wanted to couple sqlite with descriptor wallets just to avoid multiple upgrade scenarios
< sipa>
achow101: i get that, but does that add that much? you'll still need to support legacy+bdb, legacy+sqlite, desc+sqlite anyway
< wumpus>
achow101: yes, I understand … but it seems too recent to have as default yet
< luke-jr>
Guix requires running third-party blobs to use (or at least, I couldn't get it running - and if I can't, how could we expect others to?)
< achow101>
sipa: the idea was there wasn't a legacy+sqlite
< wumpus>
luke-jr: my point was, that's a concern for 0.22 not 0.21
< luke-jr>
k
< sipa>
achow101: in the short term perhaps, but longer term i'd hope you can convert a legacy bdb wallet to sqlite so we could (in due time) have a reasonable build without bdb dependency
< wumpus>
^^
< sipa>
while converting legacy to descriptor seems much harder/impossible
< achow101>
converting legacy to descriptor seems possible
< wumpus>
there needs to be a way to upgrade old wallets
< wumpus>
if not, we'll be stuck with them forever
< achow101>
my plan was for the legacy -> desc upgrade is the bdb -> sqlite upgrade too
< luke-jr>
we *could* disable descriptor wallets in 0.21, but I don't think anyone wants to go there..
< sipa>
luke-jr: yeah, no
< wumpus>
you can't really require people to transfer their funds to a new wallet to upgrade, ever
< sipa>
achow101: i see, right, if you require a new backup at the same time, it does seem reasonaBLE
< wumpus>
requiring a new backup is fine
< wumpus>
just be clear about it
< achow101>
wumpus: no funds transfer is required for the upgrade
< wumpus>
achow101: good, that's the only hope we can get rid of berkeleydb at some point :)
< dongcarl>
luke-jr: We can talk more about guix on #bitcoin-builds, would very much like to know the problems you ran into
< luke-jr>
dongcarl: k, after the meeting
< wumpus>
(which I fully support, FWIW, though we'll want to be careful to somehow have some tools somewhere that people can use to convert old bdb wallets they find)
< wumpus>
it highlights how important backwards compatibility is for our wallet formats anway
< wumpus>
any other topics?
< sipa>
42 is a good minute to end at
< wumpus>
#endmeeting
< lightningbot>
Meeting ended Thu Oct 1 19:42:43 2020 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)
< cfields>
sipa: scared to click.. don't want to click, don't want him to sue me for using pi :p
< sipa>
cfields: it's from 1972
< sipa>
hebasto: based on earlier comments by (presumably) the same user under a different identity (long story), i suspect he is using 0.20.1 and not master
< cfields>
whoops, some c/p there someow.
< cfields>
sipa: that was a joke.
< sipa>
cfields: obviously
< sipa>
sorry, should have used an interrobang
< hebasto>
sipa: thanks
< cfields>
heh
< sipa>
a c/p schnorr?
< cfields>
nm
< cfields>
I'm going to resist going down another number theory rabbit hole. This stuff is so freaking fun to learn about.