< bitcoin-git>
[bitcoin] fanquake opened pull request #18534: test: skip backwards compat tests if not compiled with wallet (master...skip_feature_compat_no_wallet) https://github.com/bitcoin/bitcoin/pull/18534
< fanquake>
wumpus / sipa can you block RANDALL-hub
< sipa>
fanquake: done
< bitcoin-git>
[bitcoin] fanquake opened pull request #18535: build: remove -Qunused-arguments workaround for clang + ccache (master...dont_quash_unused_driver_arguments) https://github.com/bitcoin/bitcoin/pull/18535
< aj>
huh, when trying to reference a pr in github, you can type "#" then some text from the topic and press enter on the one you want, and it'll replace the text you typed with the correct pr number. convenient!
< fanquake>
very
< wumpus>
oh good find!
< wumpus>
I guess it's time to branch off and wrap up 0.20.0rc1 today
< instagibbs>
\o/
< luke-jr>
wumpus: #18192 has some potential improvements, but also 3 ACKs, so hesitant to touch it - seems better to follow up with the rest in another PR
< wumpus>
ryanofsky: maybe revert it after the 0.20 branch? then go forward with your PR on master
< wumpus>
I mean, it still makes sense as a refactor
< ryanofsky>
i don't know how things are normally done. i'd revert it on master for but some reason bitcoin projects treats reverts as more of a big deal than other projects i've worked on
< wumpus>
but does your PR solve the original issue that #18338 solved, too?
< wumpus>
it seems that not fixing that issue at all (in master, at least) is strictly worse
< ryanofsky>
no, my pr is really just a refactor that doesn't change bitcoin behavior. #18338 is an actual bugfix but the implementation relies on behavior only implemented in new boost versions
< ryanofsky>
if you want to fix both reverting won't work, but the issue 18338 fixes is a crash that happens when loading and unloading the same wallet repeatedly in a loop
< ryanofsky>
the issue 18338 causes is a hang on shutdown with ctrl-c, though only with old boost versions
< ryanofsky>
reverting might not be appropriate here because we rarely revert
< wumpus>
yes, so I think reverting the fix for the weird corner case on 0.20 is acceptable to solve the more serious issue there, but if your refactor (which will go into master) solves the ensuing issue I don't see the point of reverting it on master too
< wumpus>
alternatively we should just merge your PR into 0.20 too
< wumpus>
I mean it has ACKs, more than some other 0.20-tagged things
< ryanofsky>
yep, either way is reasonable
< fanquake>
ryanofsky: you don’t necessarily need to be loading and unloading in a loop. The wallet crash can happen with just a wallet close/unload.
< wumpus>
promag: I have a slight preference to solve the problem instead of burying it that way
< promag>
when you say "solve" you mean ditch boost
< wumpus>
in the long run but I really meant merging ryanofsky 's refactor/fix
< promag>
because the problem is solved in boost>1.58
< wumpus>
but we have that PR already that fixes the problem
< promag>
yeah, I ack that. just saying that we can hit this problem again in other boost signals2 usage
< wumpus>
then let's move away from signals2 use
< wumpus>
I think there's already been movement in that direction in other places
< promag>
sure sgtm
< wumpus>
for example, another thing people have een complaining about boost::signals2 is terribly noisy gdb backtraces
< wumpus>
I only asked why #18524 was added to 0.20 because I was genuinely suprised a refactor was added, that was cleared up quickly, I like the PR itself, I don't particularly think we need an alternative solution
< bitcoin-git>
bitcoin/master 25e03ba Jon Atack: test: relax bumpfee dust_to_fee txsize an extra vbyte
< bitcoin-git>
bitcoin/master 425a7f9 MarcoFalke: Merge #18516: test: relax bumpfee dust_to_fee txsize an extra vbyte
< bitcoin-git>
[bitcoin] MarcoFalke merged pull request #18516: test: relax bumpfee dust_to_fee txsize an extra vbyte (master...relax-dust_to_fee-test) https://github.com/bitcoin/bitcoin/pull/18516
< MarcoFalke>
> [04:56] <luke-jr> Is there a reason we don't gitian-sign each individual file in addition to the packaged/tar files?
< MarcoFalke>
The tar is a concatenation of the individual files, so the signature on it includes the individual files already
< MarcoFalke>
> [06:25] <luke-jr> right now, we have zero way for users to verify snap installs
< MarcoFalke>
^ So this is not true
< luke-jr>
MarcoFalke: short of redownloading and comparing the tarball binaries..
< luke-jr>
also see our website
< luke-jr>
"While the Snap packages use the deterministically generated executables, the Snap tool itself does not provide a streamlined way to reveal the contents of a Snap package. Thus, the Bitcoin Core project does not have the information necessary to help you verify the Bitcoin Core Snap packages."
< bitcoin-git>
[bitcoin] practicalswift opened pull request #18539: Avoid using locale-dependent boost trim functions in RPCAuthorized(…) and bitcoin-tx (master...avoid-locale-dependent-trim) https://github.com/bitcoin/bitcoin/pull/18539
< wumpus>
can we have some more review on #18484 please
< gribble>
https://github.com/bitcoin/bitcoin/issues/18484 | rpc: Correctly compute redeemScript from witnessScript for signrawtransaction by achow101 · Pull Request #18484 · bitcoin/bitcoin · GitHub
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #18541: rpc: Make verifychain default values static, not depend on global args (master...2004-rpcStaticDefaults) https://github.com/bitcoin/bitcoin/pull/18541
< wumpus>
thanks! it's a one-line change, apart from test changes
< MarcoFalke>
PSA: If you see an (intermittent) test failure, please open a bug report with a link to the log.
< MarcoFalke>
Just resetting the build will make all intermittent failures accumulate and put the test suite in an unusable state where all failures are ignored and just re-run.
< sipa>
MarcoFalke: ack
< sipa>
(i occasionally did reset build failures)
< bitcoin-git>
[bitcoin] theStack opened pull request #18544: net: limit BIP37 filter lifespan (active between 'filterload'..'filterclear') (master...20200406-net-limit_bip37_filter_lifetime) https://github.com/bitcoin/bitcoin/pull/18544
< wumpus>
MarcoFalke: I generally do that, unless it's something clearly travis related like a timeout in fetching packages
< bitcoin-git>
[bitcoin] jonatack opened pull request #18545: test: refactor rpc_signrawtransaction and add logging (master...refactor-rpc_signrawtransaction) https://github.com/bitcoin/bitcoin/pull/18545
< wumpus>
as it's often is
< wumpus>
luke-jr: I agree with jnewbery's comment, if there's an actual error return mechanism that's better than asserting
< wumpus>
I think asserts (and other "crash the program now") should be used in cases the client is really in a unrecoverable, buggy state, I'm not sure that's the case here
< luke-jr>
well, it's a code error if we get there
< bitcoin-git>
bitcoin/master b86cd15 Luke Dashjr: scripted-diff: Wallet: Rename mapAddressBook to m_address_book
< bitcoin-git>
bitcoin/master 144b2f8 Luke Dashjr: Wallet: Require usage of new CAddressBookData::setLabel to change label
< bitcoin-git>
bitcoin/master 65b6bdc Luke Dashjr: Wallet: Add CAddressBookData::IsChange which returns true iff label has ne...
< luke-jr>
doh
< luke-jr>
I had just done that change >_<
< bitcoin-git>
[bitcoin] MarcoFalke closed pull request #18192: Bugfix: Wallet: Safely deal with change in the address book (master...bugfix_addressbook_change) https://github.com/bitcoin/bitcoin/pull/18192
< MarcoFalke>
Whoopsie
< MarcoFalke>
Missed the message on IRC
< MarcoFalke>
At least the three ACKs weren't invalidated 😅
< bitcoin-git>
[bitcoin] luke-jr opened pull request #18546: Bugfix: Wallet: Safely deal with change in the address book [part 2] (master...bugfix_addressbook_change) https://github.com/bitcoin/bitcoin/pull/18546
< bitcoin-git>
bitcoin/master fa2251d MarcoFalke: test: Use one node to avoid a race due to missing sync in rpc_signrawtrans...
< bitcoin-git>
bitcoin/master abc145c MarcoFalke: Merge #18543: test: Use one node to avoid a race due to missing sync in rp...
< bitcoin-git>
[bitcoin] MarcoFalke merged pull request #18543: test: Use one node to avoid a race due to missing sync in rpc_signrawtransaction (master...2004-testFixes) https://github.com/bitcoin/bitcoin/pull/18543
< bitcoin-git>
[bitcoin] MarcoFalke closed pull request #18507: test: Check that calling walletpasshprase does not freeze the node (master...2004-qaWalletFreeze) https://github.com/bitcoin/bitcoin/pull/18507
< MarcoFalke>
ok, we fixed all bugs! Ship it!?!
< sipa>
MarcoFalke: do we use -use_value_profile=1 anywhere in fuzzing?
< MarcoFalke>
sipa: It is a run-time flag you can enable when searching for seeds. It doesn't help when merely iterating over existing seeds (this is what travis is doing)
< sipa>
MarcoFalke: sure
< sipa>
but the seeds we have in in qa-assets directory, are they produced with that option enabled?
< MarcoFalke>
I didn't know the option exists until you told me about it ;)
< sipa>
it seems oss-fuzz uses it for a percentage of their runs
< sipa>
and always when -merge=1'ing
< MarcoFalke>
oh, hmm
< MarcoFalke>
We don't so we should maybe fix that: use_value_profile
< sipa>
with the fuzzer for asmap i've been working on, it seems that without that option, you very quickly converge on a set of seeds where no NEW lines appear anymore (or very rarely)
< sipa>
my intuition is that for actual testing this option isn't very useful
< MarcoFalke>
For some reason I can only copy one thing with the clipboard and then have to restart :thinking:
< sipa>
but it may help find "intermediate" fuzzer inputs that have a better chance of being mutated into useful ones
< MarcoFalke>
Yes, I suspect it should help with structs that have state and already full line coverage
< sipa>
the default feature set tracker (without -use_value_profile) is already much wider than just coverage in the typical sense, i believe
< sipa>
use_value_profile expands it further
< luke-jr>
ugh, moving on, I'm beginning to question that address book bugfix (it's still safe/good, but not something we can rely on really)
< MarcoFalke>
Just used use_value_profile on a toy example and it seems to slow down the fuzzing and make it take longer to find my hidden crash. Obviously my toy example isn't representative, but it shouldn't be enabled for all searches.
< bitcoin-git>
[bitcoin] luke-jr opened pull request #18550: Store destdata for change in separate key for backward compatibility (master...changedata) https://github.com/bitcoin/bitcoin/pull/18550
< luke-jr>
^ this feels like it really needs tests, but I'm not sure what the best way to do that is, since it's all about compatibility with older versions