< gmaxwell>
gwillen: there are a lot of reasons things can be slow.
< gwillen>
yeah, but I know I have a decent CPU, and I know how to check if my own connection is working
< gmaxwell>
including, for example, that when blocks arrive out of order, they'll all processed at once while not emptying the buffers on connections, which are only 5MB in size.
< gmaxwell>
so when a peer or two is slow and pauses validation, once the missing blocks show up, you'll connect up to 1000 blocks at once, and all your transfers stall out.
< gwillen>
oof, *nod*
< gmaxwell>
and even on a fast computer, 1 second per block to validate during sync isn't too unrealistic.
< phantomcircuit>
gmaxwell, hmm that seems kind of broken
< phantomcircuit>
how difficult would it be to process one block at a time instead? what kind of negative performance could that cause
< gmaxwell>
the reason it processes all at once is more about not ending up in a weird place during a reorg, rather than anything performance related.
< sipa>
phantomcircuit: we should just reduce the size of the download window later in the chain
< sipa>
it actually releases cs_main briefly in between each block
< luke-jr>
or finally get on with a block weight limit decrease <.<
< gmaxwell>
sipa: it releases cs_main, but doesn't exit the message handling loop and go empty the queues for the other peers.
< gmaxwell>
it's really easy to see the effect... bandwidth usage continually drops to zero while syncing.
< sipa>
gmaxwell: yup
< sipa>
the solution to that is doing blocm validation in the background...
< gmaxwell>
or just exit the message handling loop after processing a couple blocks... so that it can go drain the queues for the other peers.
< sipa>
fair; but we'd need to make sure to not process any further messages from the peer until all blocks are processed
< sipa>
but that doesn't sound too hard actually
< gmaxwell>
Some people, when confronted with a problem, think "I know, I'll use concurrency." Now they have tproblemsow.
< sipa>
Some people, when confronted with a problem, think "I know, I'll use floating point." Now they have 1.9999997 problems.
< andytoshi>
in fairness, i can read "tproblemsow" twice as fast as i could read two words
< * andytoshi>
ducks
< phantomcircuit>
gmaxwell, hmm i guess with a reorg we rewind and then walk all the way to the new tip
< phantomcircuit>
could rewind to the split and then do normal sync again with the new headers
< phantomcircuit>
i assume rewinding is reasonably fast?
< phantomcircuit>
or no
< gmaxwell>
rewinding is slow
< sipa>
phantomcircuit: we release cs_main whenever we're in a state that's better than the previous state
< sipa>
or when we're back in a previous state and there is no valid better chain available
< gmaxwell>
it should just do enough work to get to a better position then it last was and stop.
< gmaxwell>
so a reorg would still block things for a while but otherwise it won't slow down progress generally.
< phantomcircuit>
sipa, it would be nice if there was something like "step and tell me if you did something"
< phantomcircuit>
then run that in the background
< sipa>
gmaxwell: seems reasonable... but how do you make sure to continue processing blocks when no peer is giving you anything?
< sipa>
whenever the processing timeout fires?
< phantomcircuit>
sipa, background thread
< sipa>
phantomcircuit: i'm trying to get gmaxwell to see that may be useful :p
< meshcollider>
hebasto: in my opinion not really :)
< wumpus>
ooh time to upgrade OpenBSD
< meshcollider>
sipa: still trying to figure out why the P2WSH address is not IsMine, I've confirmed both private keys and the witness script were imported into the wallet successfully, and it works for P2SH-P2WSH, so I'm very confused
< meshcollider>
Its solvable, and both private keys are there, so what more does it need :(
< meshcollider>
I'm either missing something really dumb, or there's a bug in the IsMine logic
< meshcollider>
actually I think I know the issue, let me test something
< meshcollider>
sipa: yes, the scriptPubKey also needs to be added to the wallet scripts as well for it to be IsMine, not just the witnessScript so it works now
< harding>
Re: possibly removing the GUI addressbook, I just wanted to note that I use that with my cold wallet setup so that I can import a handful of watch-only pubkeys at a time using the RPC (labeling them as, e.g., "cold-2019-10-19") and then use the GUI addressbook to hand them out as necessary over the next few weeks (relabeling them as I use them to both track payments and prevent address reuse). I could probably do the same thing
< harding>
just as easily using a text file, so I wouldn't mourn the loss of the addressbook, but it is something that's currently a part of what I think is a reasonable workflow. The Receive tab doesn't currently show imported stuff until you receive payment to it.
< wumpus>
harding: I think that's a reasonable workflow, too
< wumpus>
gah openbsd 6.4 upgrade failed here, some error at startup of the kernel, might have to do with the specific qemu config
< SigmaOC>
REPLY isn't just a zsh thing. Works in any shell; it's just the convention (dunno where it started).
< SigmaOC>
PuppyNews_, I'm afraid no one from wikipedia can hear you right now, this channel is moderated (+m)
< soahccc>
malloc/realloc/free is just a mistake while dealing with C++ code
< bitcoin-git>
[bitcoin] promag opened pull request #14518: rpc: Always throw in getblockstats if -txindex is required (master...2018-10-getblockstats) https://github.com/bitcoin/bitcoin/pull/14518
< bitcoin-git>
[bitcoin] practicalswift closed pull request #13971: Add tests and error handling to DecodeExtPubKey/DecodeExtKey. Add [[nodiscard]]. (master...DecodeExtKey) https://github.com/bitcoin/bitcoin/pull/13971
< bitcoin-git>
[bitcoin] practicalswift closed pull request #13969: Make sure all callers of LookupBlockIndex(...) check for nullptr before dereferencing (CBlockIndex*) (master...LookupBlockIndex) https://github.com/bitcoin/bitcoin/pull/13969
< jonasschnelli>
promag: is it possible to add a test for 14453?
< bitcoin-git>
[bitcoin] jamesob opened pull request #14519: test: add utility to easily profile node performance with perf (master...2018-10-func-test-profiling) https://github.com/bitcoin/bitcoin/pull/14519
< midnightmagic>
wumpus: I didn't know you were into OpenBSD. Bitcoin builds work okay on it still? There was a guy who was persistently building on OpenBSD for quite some time.
< sipa>
meshcollider: argh, indeed, i should have known
< sipa>
welcome to the first bitcoin core wallet meeting!
< meshcollider>
\o/
< sipa>
i think it would be good to start with listing what people are working on
< gmaxwell>
I am working on nothing wallet related. (there, now everyone doesn't have to worry, since you can't be doing worse than nothing)
< meshcollider>
I've been working on getting importmulti working with all the segwit address types and everything recently, and trying to get through most of sipa's descriptor PRs for review :)
< gwillen>
oh hm, I am glad I happened to see this but I am interested in how one finds out about these meetings and their topics :-)
< sipa>
gwillen: discussed yesterday in the bitcoin core meeting :)
< sipa>
i'm working on a number of smaller descriptor related improvements, before digging into fully importing descriptors into the wallet
< meshcollider>
Should we ping the list of Devs from the main meeting in case anyone who wanted to be here forgot
< gwillen>
Wallet-wise: I am still working on the offline signing usecase, modeled after the flow armory uses with gui interfaces for create-sign-broadcast.
< kanzure>
gwillen: also it was discussed in tokyo. you were there.
< gwillen>
kanzure: like I remember things people say.
< luke-jr>
lol
< jamesob>
anyone know of an easy way to clear the sig/script caches of a running bitcoind process?
< achow101>
hi
< sipa>
jamesob: i'm not sure it's possible
< jamesob>
oops! sorry to interrupt the meeting :)
< sipa>
jamesob: but meeting now :)
< luke-jr>
btw, not wallet related, but: my node stats are b0rked for a bit :x
< sipa>
yay, we have some people
< kanzure>
are descriptors going to be renamed (or descript?)
< luke-jr>
sorry, gtg
< meshcollider>
kanzure: why would they be renamed?
< sipa>
kanzure: we've sort of settled on the name miniscript instead of descript; descriptors are descriptors :)
< achow101>
meshcollider: because it's confusing
< sipa>
(they're much less related to eachother than the name seemed to imply anyway)
< kanzure>
miniscript because it's yer subset of script. alright.
< kanzure>
i would have also accepted sipascript
< meshcollider>
achow101: descriptors is a good name? Only "descript" might have been a little ;)
< achow101>
so what steps do we need to do to get the wallet to be descriptor based?
< gwillen>
kanzure: I have also been pushing for "output descriptors" rather than "script descriptors" given the choice of both, the latter being a bit of a tongue twister :-)
< sipa>
after that a number of things become possible, including #14477, the ability to add origin info to scantxoutset (which would solve #14503), and it would also allow writing a descriptor/utxoset based PSBT updater
< sipa>
achow101: yeah, that's the big question :)
< sipa>
i think the first step is abstracting out IsMine
< gmaxwell>
scanutxoset based psbt updater would be a major win.
< kanzure>
need to keep old ismine things?
< sipa>
kanzure: i would prefer that, and i also don't think it's that much extra work
< sipa>
IsMine is really simple to add things too - just OR the result
< meshcollider>
Long term, is making the wallet descriptor-based enough to solve all the existing concerns with same keys being used for different address types and which keys we treat as IsMine, etc?
< sipa>
meshcollider: yup
< achow101>
sipa: isn't IsMine already separated from the wallet?
< sipa>
so one way of seeing it i think is that a wallet will consist of a number of records, each of which has one descriptor plus some metadata (birthdate, change or not, explored how far, gap limit, ...), and one record is designated "here is where you draw payment addresses from" and "here is where you draw change from"
< sipa>
and then there can be - for now - a 'legacy' record that corresponds to the behaviour of the existing keypool/mapkeys/mapwatchonly/mapscripts
< sipa>
achow101: so the goal would be that there can be multiple implementations of the ismine logic, and one (the legacy one) needs to encapsulate the keypool logic... which is a nontrivial change
< sipa>
right now IsMine is just something that operates based on a KeyStore, and that's not enough
< meshcollider>
For the payment and change addresses sources, they would just be ranged descriptors too right?
< sipa>
yup
< sipa>
another missing piece is an "evaluation cache" for descriptors, which would e.g. store pubkeys for hd keys which have hardened steps in them
< meshcollider>
Store pubkeys for solvabilty while the wallet is locked, or?
< sipa>
meshcollider: well, and to compute the scriptPubKeys to watch for
< sipa>
for efficiency maybe it shoud store all pubkeys involved in descriptors, even the ones from unhardened paths
< sipa>
or we'd need to rederive them at startup
< sipa>
probably the first step is creating an interface for IsMine; just like SigningProvider is an interface for solving/signing
< sipa>
initially the wallet itself can implement that, by calling the old IsMine code, but that code can then be moved into one instance of that logic
< sipa>
after that, it should be easy to create another descriptor based implementation
< sipa>
</monologue>
< sipa>
other topics?
< meshcollider>
Upgrade wise, descriptor based wallets aren't backwards compatible with old versions, or we are going to allow some more magic not-touching-the-wallet derivation of descriptors at startup every time?
< sipa>
yeah, that's a different question; i think there are a number of ways
< sipa>
one is to keep them completely separate, and old wallet remains compatible with old versions as long as you don't import anything descriptor based
< sipa>
another is to convert the existing stuff to descriptors at startup every time - which would allow removing the existing IsMine logic from the runtime, but not gain us much otherwise (and the conversion is pretty complicated)
< achow101>
it also wouldn't work for encrypted wallets
< sipa>
and i guess another is to just have 2 types of wallets, and you need an explicit conversion between them
< sipa>
achow101: i think it would
< sipa>
we know the xpub we derived keys from, no?
< achow101>
no
< achow101>
xpub and xprv are derived from the seed on the for whenever they are needed
< sipa>
no?
< sipa>
ah
< achow101>
alao hardened derivation, so no xpub
< sipa>
that's ok
< sipa>
but yeah
< sipa>
i think it's actually best to at least initially have the two live side by side
< meshcollider>
Can we deprecate the old version and in a future release only accept wallets if they've been upgraded?
< sipa>
perhaps
< achow101>
I'm afraid that if we have to keep the old one around that it will never go away because people don't upgrade
< meshcollider>
That's what I mean
< sipa>
i think a bigger question is what to do with things like addmultisigaddress etc
< achow101>
can't it construct a descriptor based on what was given?
< meshcollider>
Isn't that ok to just make a new multisig descriptor record with the public keys of whatever was specified?
< gmaxwell>
that was my thought
< sipa>
achow101: yeah, with slightly different (but far more reasonable, actuallly) semantics
< sipa>
meshcollider: same with importmulti, actually
< sipa>
anything specified there can be converted to a descriptor
< andytoshi>
meshcollider: people today show up with 2012 wallets on #bitcoin, and it's a pretty impressive show of backward-compatibility that i can always say "just import it into the latest core"
< andytoshi>
even if it's got e.g. p2pk outputs
< booyah>
andytoshi: yeap, I think it's safe to assume people will expect that basically forever. Wallets in some deep storate boxes, inherited, burried in backyard
< meshcollider>
andytoshi: you could still import it, but it would upgrade it for you I think
< meshcollider>
So you just can't import it and then go back and use it on the 2012 software
< sipa>
i think that the burden of maintaining compatibility with the old wallet format actually won't be too hard, as due to the necessary refactoring for descriptors in the first place, it will become pretty standalone and not entangled with everything else
< sipa>
and i shouldn't say "old format", it's just adding some new fields
< jonasschnelli>
I still sometimes have the feeling we should clone the wallet code (make it run with the existing wallet code) and remove everything that is legacy,.. don't promise backward comp. for 1-2 yrs.
< jonasschnelli>
All the fancy stuff could go there...
< jonasschnelli>
API can break during that 1-2yr period
< sipa>
jonasschnelli: maybe, but i think that's not the right granularity
< sipa>
you'd be duplicating a lot of things that are perfectly compatible
< sipa>
like coin selection
< jonasschnelli>
We still can "backport" to the stable wallet
< jonasschnelli>
You can factor out the coin selection
< sipa>
exactly.
< jonasschnelli>
But I agree, some parts would. be duplicated
< sipa>
just like you can factor out the ismine logic.
< sipa>
:)
< gmaxwell>
I don't think we get enough testing/review for one wallet, having two doesn't sound better. :) if it were narrowed down to a pretty small thing, then sure.
< gmaxwell>
Being able to _import_ old wallets should always be relatively straight forward though, so it may make sense to not support old wallets except being able to import them at some point.
< sipa>
yeah, agree
< jonasschnelli>
I think we test less if we go with the two-wallets approach since backward compatibility and the edge-cases that come up with it consumes a major part of the review and fix time
< sipa>
i feel that "meh just create a new wallet" is a knee jerk response when facing the complexity of the existing system
< jonasschnelli>
At some point in time, you have to abandon old wallets (maybe thats not within the next 10 yrs)
< sipa>
it's always appealing to rewrite things you don't understand
< sipa>
but that doesn't make it the right choice
< jonasschnelli>
Yes. Maybe.
< gmaxwell>
And usually a bad idea, since the complexity was almost always there for a reason. :)
< sipa>
it is certainly true that _some_ of the complexity can be dropped if we don't need backward compatibility
< sipa>
but identifying that is perhaps less work than making a new implementation actually production ready
< jonasschnelli>
I don't think its about complexity... more about progress we can't make otherwise. And dragging around legacy stuff like the account system.
< sipa>
i think we can make progress fine
< jonasschnelli>
Yes. That indeed true.
< sipa>
especially with a number of people pulling in the same direction - which is something that hopefully these meetings contribute to
< jonasschnelli>
As said, somethimes I think it would be worth do the 2nd wallet... but you guys always convince me the single-wallet approach is more future proof
< meshcollider>
Is #8369 in any way helpful to this discussion
< gribble>
https://github.com/bitcoin/bitcoin/issues/8369 | [FOR LATER USE][WIP][Wallet] add support for a flexible "set of features" by jonasschnelli · Pull Request #8369 · bitcoin/bitcoin · GitHub
< jonasschnelli>
meshcollider: oh. I think we implemented that in a way,.. not?
< jonasschnelli>
With the disableprivatekey function
< jonasschnelli>
The disableprivatekey function introduced a 64bit bitmap
< jonasschnelli>
Oh. I smell that this is a good read. Thanks sipa
< jonasschnelli>
meshcollider: with that 64bit features flag bitmap, the upper 32 are mandatory (wallet needs those features) where the lower32 bits are optional.
< sipa>
jonasschnelli: i'm in favor of something like that, though we shouldn't overuse it as it may lead to an explosion of combinations to test
< sipa>
(which is less of a concern when when the features don't interact, and another argument for abstracting out things, so that it becomes clear they can't interact)
< sipa>
oh, i didn't realize the disableprivatekey introduced that already
< meshcollider>
A descriptor wallet could just be a mandatory flag then right
< jonasschnelli>
Yes. I smuggled it in
< sipa>
meshcollider: right
< jonasschnelli>
Things like disableprivatekeys is optional and needs a such facility
< sipa>
i guess that's it?
< meshcollider>
Alright this has been a good meeting IMO, last couple minutes anything else?
< meshcollider>
Maybe it's just been good because I've been asking lots of questions lol
< jonasschnelli>
We should use the meetingbot I guess?
< jonasschnelli>
(next time)
< meshcollider>
Well, it's running
< jonasschnelli>
Oh. We did.
< meshcollider>
Just no explicit actions or topics
< jonasschnelli>
I see. Sure.
< sipa>
#endmeeting
< lightningbot>
Meeting ended Fri Oct 19 20:00:03 2018 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)
< jarthur>
jnewbery wumpus any known issues with the new wallet and walletdir behaviors in 0.17? Someone in #bitcoin is saying the backwards compatibility with prior behavior mentioned in release notes isn't working for them.