< emilengler>
Is there a cmake file for bitcoin core?
< sipa>
emilengler: it's an autotools project
< fanquake>
hebasto: looks like ipglider hasn't been on GH for a while. I'm going to close #15768 as "Up for grabs", feel free to pick it up if interested.
< provoostenator>
For todays wallet meeting, it would be good to brainstorm if there's a way to avoid all the [ci skip] commits in #16341.
< gribble>
https://github.com/bitcoin/bitcoin/issues/16341 | Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101 · Pull Request #16341 · bitcoin/bitcoin · GitHub
< provoostenator>
I can't figure out how.
< provoostenator>
It's not that I mind skipping Travis, but I would find this PR much easier to review if it moved stuff to (Legacy)ScriptPubKeyManager more incrementally.
< ariard>
at what time is wallet meeting ? I would like to get feedbacks on the rework of rescan logic I'm hacking on
< achow101>
ariard: 1900 utc
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #16535: test: Explain why -whitelist is used in feature_fee_estimation (master...1908-testDocFeeEst) https://github.com/bitcoin/bitcoin/pull/16535
< ariard>
thanks achow101
< elichai2>
Is InferDescriptor *suppose* to add this fingerprint to keys(they started as regular descriptors with private/public keys, no BIP32 stuff involved)? `pkh([1fb31c4f]03462c64aa6089c6e28536c74b6ec4a8f3eaf2f5c5c36e1ae0abf39d563eeaf11e)` (it's something i'm seeing in my descriptors_tests)
< sipa>
elichai2: it's unnecessary but harmless
< elichai2>
So it's nothing i've done haha thanks :) (FYI it's starting to look nice, this is what my InferDescriptor *returns* to me :) `tap([46d3fd75]031e34802508ce0bbabb71935832c92129c6df82143a924d731c43362495111319,[pkh([1fb31c4f]03462c64aa6089c6e28536c74b6ec4a8f3eaf2f5c5c36e1ae0abf39d563eeaf11e),pk([2611005f]0257dd0c7c2e9036b845ff4f8a90eeed5daf821e7abd1f98dcbc8b65b0006d28ce)])#umstfklz`)
< elichai2>
(I meant parsing and then inferring the scriptpubkey)
< sipa>
cool!
< bitcoin-git>
[bitcoin] ariard opened pull request #16536: [doc] Update and extend benchmarking.md (master...2019-08-update-benchmarking-docs) https://github.com/bitcoin/bitcoin/pull/16536
< bitcoin-git>
[bitcoin] MarcoFalke merged pull request #16097: Refactor: Add Flags enum to ArgsManager class (master...20190526-fix-negated-args) https://github.com/bitcoin/bitcoin/pull/16097
< elichai2>
sipa: arghh. Is it important that every bracket will have it's own significant meaning? because using the same bracket for taproot branches and for bip32 derivations complicates stuff a bit (nothing that can't be handled with a few conditions but still) should be maybe introduce curly brackets?
< sipa>
elichai2: if you think curly brackets is easier, sure
< bitcoin-git>
[bitcoin] Sjors closed pull request #15414: [wallet] allow adding pubkeys from imported private keys to keypool (master...2019/02/keypool_import_private_keys) https://github.com/bitcoin/bitcoin/pull/15414
< sipa>
TIL: the number of public keys participating in an OP_CHECKMULTISIG and OP_CHECKMULTISIGVERIFY counts towards the 201 non-push opcodes limit, but *only* when executed
< meshcollider>
#startmeeting
< lightningbot>
Meeting started Fri Aug 2 19:00:21 2019 UTC. The chair is meshcollider. Information about MeetBot at http://wiki.debian.org/MeetBot.
< ariard>
thanks to feedbacks design from ryanosfky
< sipa>
ariard: tip for remembering his nickname: ryan of sky
< ariard>
s/ryanosfky/ryanofsky/g
< ariard>
sipa: already heard this one
< meshcollider>
achow101: The 107 commits include the legacy rework PR right :p
< achow101>
<provoostenator> For todays wallet meeting, it would be good to brainstorm if there's a way to avoid all the [ci skip] commits in #16341.
< gribble>
https://github.com/bitcoin/bitcoin/issues/16341 | Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101 · Pull Request #16341 · bitcoin/bitcoin · GitHub
< achow101>
meshcollider: yes
< kanzure>
hi
< achow101>
I think provoostenator has the only topic suggestion today
< achow101>
I had a look at doing that, and it gave me a headache
< meshcollider>
#topic Avoiding the [ci skip]s in #16341
< gribble>
https://github.com/bitcoin/bitcoin/issues/16341 | Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101 · Pull Request #16341 · bitcoin/bitcoin · GitHub
< achow101>
the problem is that a lot of functions are dependent on other ones, so there would need to be a pretty big commit just to add those
< meshcollider>
Yeah I thought the whole reason you put them there in the first place was because we decided it was better than one massive commit that doesn't break stuff
< achow101>
then as you add them into the wallet, things start becoming inconsistent if not all of the things that can effect each other are also added at the same time. these inconsistencies also result in test failures
< achow101>
One idea I had was to combine implementation of the function in LegacyScriptPubKeyMan and replacement in CWallet, but still leave the CWallet stuff there to operate in parallel
< provoostenator>
That's what I was thinking as well
< achow101>
but that seemed kind of pointless because the new code wasn't really doing anything, and we would still need to have the gigantic "delete all the old things" commit at the end
< provoostenator>
Maybe start with a bunch dummy methods in the LegacyScriptPubKeyMan and then move stuff over?
< provoostenator>
Then in the end you still need to delete some calls, but no implementations.
< achow101>
there is also some consistency issues with doing that because the wallet files could be overwritten by two different code paths and become inconsistent, also failing tests
< meshcollider>
It's the moving which is the problem
< elichai2>
crazy idea. PR into PR? (that way they could be reviewed seperatly but merged into master together)
< sipa>
we must go deeper.
< achow101>
elichai2: they're all separate commits, so it shouldn't actually be that hard to review separately
< achow101>
provoostenator: I don't believe that there is a way to avoid having [ci skip] commits because tests will fail
< meshcollider>
Ease of review > Travis passing on every single commit, as long as the tests pass at the end of the PR I think it's fine :)
< provoostenator>
meshcollider: agree
< achow101>
a lot of the commits are really small too
< provoostenator>
Currently the dimmed-zebra can't help
< provoostenator>
Which makes it super tedious to check if the new function bodies are the same
< sipa>
having intermediary commits that pass CI is helpful for review, as you can be sure about things like "this method changes, did all call sites get updated too?" without needing much work
< achow101>
having them all pass tests would be nice, makes bisect not suck
< sipa>
yeah
< sipa>
not saying it's a strict requirement, though it's certainly very helpful as a policy
< achow101>
I spent a lot of time finding a bug that was introduced in one of the [ci skip] commits but bisect wasn't as helpful since the tests wouldn't get to the part where the bug was triggered
< achow101>
but even then, having them all pass tests wouldn't be helpful since the new code isn't the one that's actually doing the work. it would still just be the old stuff and bisecting would just point you to the gigantic "change everything over" commit
< jonatack>
at least the [ci skip] comments signal when the failure is to be expected, so there's that... but from a review perspective hygienic commits are much better
< jonatack>
commit messages could contain the steps
< meshcollider>
achow101: can CWallet just have an instance of a LegacyScriptPubkeyManager inside it right from the start and call methods inside it as they're moved into it?
< meshcollider>
And then do some inheritance refactoring at the end
< meshcollider>
Then the massive "move everything over" commit would just be deleting methods like foo(){ legacy.foo(); }
< achow101>
no because methods are dependent on other ones
< achow101>
for example, I thought that AddKey would be easy, it should be mostly by itself, right? Nope. Requres AddCryptedKey to be implemented, which requires Encryption stuff to be implemented. For some reason it also requires watch only things so you need to have setWatchOnly, HaveWatchOnly, RemoveWatchOnly
< achow101>
and you need AddKey for loading keys in the first place!
< sipa>
fun.
< meshcollider>
Yeah... I think the [ci skip] are here to stay then
< meshcollider>
Does anyone else want to discuss anything this week?
< achow101>
(If you couldn't tell, I was very annoyed by this and spent a while cursing the person who wrote this code, which was probably sipa :p)
< achow101>
it would be nice for rescan (and other wallet things) to not need cs_main
< meshcollider>
#action Read and feedback on ariard's proposal
< ariard>
main idea is just to use a common thread between index and ChainClient to provide blocks in one sequence instead of redoing the work multiple times
< achow101>
there's been a lot of complaints I've seen recently about RPCs taking a while, probably because cs_main is held by ~everything
< ariard>
achow101: yes that's the end of goal, not relying at all on cs_main
< ariard>
but will need multiple PRs to get there to avoid to big diff
< ariard>
first step move all logic in a thread, future steps worker thread pool + cache what we need to avoid locking
< ariard>
hope to submit code next week, people will get a better idea
< emilengler>
By the way, what does the LOCK and cs_main mean? I see this nearly everywhere when I look in the code
< meshcollider>
ariard: sounds good!
< emilengler>
And why it needs to be in the curly brackets
< achow101>
emilengler: cs_main is one of the locks, while one thread holds it, no other thread can acquire it. it is used to protect things from concurrency issues like race conditions. LOCK(cs_main) is the way to acquire cs_main, if it is held by someone else, the thread will wait
< sipa>
emilengler: RAII
< meshcollider>
emilengler: cs_main is something which only one thread can "hold" at once, making sure stuff which it protects isn't written twice at the same time or something which would result in race conditions
< sipa>
emilengler: it introduces a lock in the scope it's defined at; it automatically unlocks when it goes out of scope
< sipa>
emilengler: it's a general concept in C++ called RAII
< ariard>
have a look in sync.h and threadsafety.h
< meshcollider>
Anyway I think this wraps up the meeting
< emilengler>
Ok so it is more a C++ thing? I usually don't work that often with multiple threads
< meshcollider>
We can read the proposal and PR outside
< meshcollider>
#endmeeting
< lightningbot>
Meeting ended Fri Aug 2 19:31:49 2019 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)
< bitcoin-git>
bitcoin/0.18 fa27a07 Wladimir J. van der Laan: doc: Bump manpages pre-final
< provoostenator>
^ sorry for the bike-shed risk, but I think it's worth considering every couple of years and it's been 5
< wumpus>
emilengler: yea there's some svg but they're converted to png before being included afaik, don't think we include any actual vector icons for use by the qt code, but not 100% sure
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #16540: test: Add ASSERT_DEBUG_LOG to unit test framework (master...1908-UnitTestAssertDebugLog) https://github.com/bitcoin/bitcoin/pull/16540
< wumpus>
provoostenator: sorry i'm not sure what that's about
< provoostenator>
wumpus: my PR proposes lowering txmaxfee from 0.1 to 0.01 BTC
< wumpus>
gkrizek: ^^ the bot really does work great
< provoostenator>
With giant coin joins in mind maybe we should check the max fee per our own inputs, rather than the tx total.
< provoostenator>
*for
< wumpus>
yes, tx total has always been kind of a belts and suspenders measure
< wumpus>
like 'if the total fee is more than *this* it's really absurd and something must be wrong'
< wumpus>
could be because a wallet generates a transaction that's absurdly large, or because the feerate is absurdly large
< provoostenator>
In #16257 I covered the case where a user types "1" as the feeRate thinking it's satoshi per byte. That works nicely with the current 0.1 BTC max.
< provoostenator>
But there may be other fat finger mistakes possible.
< provoostenator>
I believe in earlier days people would sometimes forget a change address, though afaik that's pretty hard to do accidentally now in core.
< ghost43>
provoostenator: is the maxtxfee check still done when using the sendrawtransaction RPC? Not that long ago at least that was the case
< provoostenator>
I'm not sure, because that's no longer part of the wallet.
< ghost43>
(I know the check was done there because it was affecting electrum users)
< provoostenator>
ghost34: yes it still does that check, there's a maxfeerate argument to more easily override it
< achow101>
ghost43: the absurdly high fee error?
< ghost43>
yes
< ghost43>
we had users with huge wallets creating almost 100 KB txs
< provoostenator>
But it's not done for p2p transaction relay?
< ghost43>
for those, the absurdity is breached around 100 sat/byte
< ghost43>
but the electrum server is using the bitcoind sendrawtransaction RPC
< bitcoin-git>
[bitcoin] emilengler opened pull request #16541: qt: Add better icon for Open URI (master...2019-08-qt-update-open-uri-icon) https://github.com/bitcoin/bitcoin/pull/16541
< ghost43>
tbh, I think a "feerate" absurdity check would make a lot more sense than an "absolute fee" absurdity check
< achow101>
ghost43: I think it's still an absolute fee. We should definitely change that...
< achow101>
afaik maxtxfee is for the wallet
< achow101>
ghost43: nvm, I'm wrong. it uses maxtxfee. you could also just bypass that check by using the `allowhighfees` parameter
< emilengler>
Is the strprintf function for args who take user-input?
< sipa>
for anything
< emilengler>
But in init.cpp this function is used sometimes and sometimes not
< emilengler>
So what is the actual purpose of it?
< sipa>
it's used to convert things to a string
< sipa>
i don't know what you mean by "sometimes not"
< emilengler>
In the init.cpp at the part where the args are being added their is the second parameter sometimes this function or just a string
< emilengler>
Like in line 356 it isn't used but in 383 it uses for example
< sipa>
there is nothing to convert to a string in line 356
< sipa>
it already is one
< emilengler>
Yep I got it, your previous answer explained it. thanks :)
< bitcoin-git>
[bitcoin] achow101 opened pull request #16542: Return more specific errors about invalid descriptors (master...descriptor-errors) https://github.com/bitcoin/bitcoin/pull/16542
< achow101>
fanquake: maybe we should add a "descriptors" tag as descriptors by themselves aren't wallet related
< fanquake>
achow101: sure
< elichai2>
achow101: why doesn't `FillableSigningProvider` derives from `FlatSigningProvider`?