< bitcoin-git>
[bitcoin] JimmyMow opened pull request #15802: doc: create application support bitcoin folder (master...fix/macos-docs) https://github.com/bitcoin/bitcoin/pull/15802
< bitcoin-git>
[bitcoin] meshcollider opened pull request #15803: [0.18] Backport 15749: importmulti only imports origin info for PKH outputs (0.18...201904_backport_15749) https://github.com/bitcoin/bitcoin/pull/15803
< bitcoin-git>
[bitcoin] practicalswift opened pull request #15805: log: Increase signal-to-noise in bitcoind standard output. Don't print debug output "Pre-allocating to position ..." and "Leaving block file ..." when running with -nodebug (default). (master...stdout-signal-to-noise) https://github.com/bitcoin/bitcoin/pull/15805
< bitcoin-git>
[bitcoin] practicalswift opened pull request #15806: contrib: Remove SUSPICIOUS_HOSTS from makeseeds.py (master...remove-SUSPICIOUS_HOSTS) https://github.com/bitcoin/bitcoin/pull/15806
< MarcoFalke>
All tests are busy printing at least every 10 minutes, so the "no output seen" should never be hit
< jnewbery>
I think there's a wallet meeting in just over an hour today. If so, I have one #proposedmeetingtopic : Upgrade wallet (#15761). There's already been some discussion in the PR
< jnewbery>
This PR is to replace the -upgradewallet startup option, which is great news
< jnewbery>
achow is proposing to replace it with an RPC. There were a couple of other suggestions on what to replace it with in the PR and I wanted to poll for opinions
< achow101>
I think the primary question is RPC, wallet-tool command, or both
< achow101>
I prefer both
< jnewbery>
I'm slightly concerned about having an RPC that can upgrade at any time. I just feel that it might introduce subtle corner cases if the wallet is doing something else at the time
< jnewbery>
but I might be wrong. Perhaps it's fine
< achow101>
the whole operation is locked, and any operations on the wallet are also locked, so I don't think that's really a problem
< jnewbery>
(I'm definitely a big concept ACK on moving away from startup option)
< kanzure>
does upgradewallet replace the wallet or does it create an upgraded wallet only?
< achow101>
kanzure: it just upgrades the wallet file
< luke-jr>
could be only allowed when loading
< meshcollider>
I'm in favour of both too
< jnewbery>
luke-jr: that's one suggestion in the PR. Another option would be to only allow it on wallets that aren't currently loaded.
< jnewbery>
(ie separate upgrade from running entirely)
< achow101>
jnewbery: what if upgradewallet unloads it, upgrades, then reloads it
< achow101>
that would disconnect all of the signals and stuff that would cause issues
< kanzure>
need lock during upgrade
< kanzure>
so that they don't reopen
< kanzure>
in achow101's flow.
< jnewbery>
Should we encourage users to backup before they upgrade?
< meshcollider>
kanzure: I think thats the current behavior anyway
< kanzure>
meshcollider: ok, i thought the lock only applies to loaded wallets. nevermind.
< achow101>
kanzure: meshcollider: in the idea I just proposed, you would have to lock something to prevent loading the wallet during upgarde
< luke-jr>
jnewbery: at least after
< harding>
Wallet files are generally small. If you think a backup is important, it's probably better to just make one automatically and stuff it somewhere in ~/.bitcoin/
< achow101>
jnewbery: probably. the help text for the RPC does say that backups after are required
< achow101>
I can change it to also say before too
< meshcollider>
backup before can be done automatically like harding suggests
< meshcollider>
Make a wallet.old or something
< harding>
(Backup after is a different thing, since that's talking about off-disk backups.)
< luke-jr>
achow101: well, only after is *required*..
< achow101>
right
< achow101>
I think it's possible to make both an upgradewallet RPC and wallet-tool command work safely
< meshcollider>
It doesn't seem like theres any real objection to that
< meshcollider>
jnewbery: anything else to discuss on this topic?
< achow101>
the only question I really have is what to do with the tests for the old wallet stuff
< jnewbery>
I think it's pretty important to keep those tests
< achow101>
currently I have a bunch of tests removed or changed because they don't work with descriptor wallets
< jnewbery>
Most users will still be using non-descriptor wallets for some time. We can't just stop testing those and hope there are no regressions
< ryanofsky>
you can add descriptors as an optional feature, there's no need to remove old code or old tests
< achow101>
ryanofsky: that's an option, but I feel like descriptors and its new definitions is such a departure from current wallet stuff that it should have better distinction than just a wallet flag
< ryanofsky>
you are referring to the practical downside of having to keep more code around?
< achow101>
e.g. it's possible for someone to write code which accidentally unsets the wallet flag. it's much harder to accidentally downgrade the version
< meshcollider>
Does your PR remove the ability to generate new "old" wallets
< achow101>
meshcollider: yes
< achow101>
(it's a wallet version bump)
< sipa>
achow101: i think, even for just testing purposes, we'll need to retain the ability to create old wallets
< jnewbery>
The main downside of it being a flag rather than a version is that it becomes combinatorially more difficult to test everything
< sipa>
as this is a very invasive change, and i don't think we want to lose the ability to test old logic
< jnewbery>
I think that the default should be that newly created wallets are old-style, and users need to explicitly upgrade
< achow101>
I think an explicit upgrade is actually far more dangerous than creating a new descriptor wallet
< jnewbery>
because like you say, it's a big departure from the current wallet design
< jnewbery>
we need to support both for some time to come, so why not take the conservative approach for now
< meshcollider>
I think thats sensible for now, old wallet generation can be "deprecated" later on
< ryanofsky>
yeah, i just think you don't need to add descriptors as this big one time change
< jnewbery>
> I think an explicit upgrade is actually far more dangerous...
< ryanofsky>
you can add new functionality alongside existing functionality, you will get better review better testing
< jnewbery>
We definitely shouldn't be releasing wallet code that we think is *in any way* dangerous
< achow101>
ok
< achow101>
jnewbery: the upgrade stuff I think will be inherently dangerous. shoehorning the old ismine logic into the new ismine logic is not trivial and they are incompatible in many different ways
< ryanofsky>
achow101, you don't even need to add upgrade in the initial pr
< ryanofsky>
we can add support for just creating new descriptor wallets, or just importing descriptors first
< jnewbery>
i don't agree that it's inherently dangerous. We just need to do lots of testing until we're satisfied that it's no longer dangerous
< meshcollider>
provoostenator is having IRC issues and can't send messages here atm but would like to point out he also has a slightly different (and perhaps less complete) PR open: https://github.com/bitcoin/bitcoin/pull/15487
< jnewbery>
and roll it out slowly, with warnings to backup, etc
< achow101>
ryanofsky: right
< jnewbery>
Everything ryanofsky is suggesting is still possible by using wallet versions and not flags
< achow101>
ryanofsky: we shouldn't allow people to create mixed descriptor and non-descriptor wllets though. so no importing descriptors
< achow101>
jnewbery: making it optional is not
< achow101>
that should only be done with flags
< jnewbery>
why not? Add a new parameter to createwallet
< ryanofsky>
yeah, i'm not sure you actually need a version or a flag
< jnewbery>
if descriptorwallet=false create an old-style wallet, if true create a descriptor wallet
< achow101>
the last time we did optional version was hd wallet and that was a headache to reconcile with upgrades in the future. I would rather not go through that excercise again
< jnewbery>
default to false
< jnewbery>
we need to handle upgrades anyway. You already have code to do that
< provoostenator>
hi?
< harding>
provoostenator: hi
< provoostenator>
YES!
< meshcollider>
provoostenator: hi \o/
< jnewbery>
hi sjors!
< provoostenator>
That was weird, I've been talking into a void for a day or so :-)
< provoostenator>
In my version of descriptor wallets it's a feature flag and opt-in. Obviously this sort of thing is easy to tweak.
< achow101>
jnewbery: I think we should maintain the separation of wallet flags for optional, wallet version for mandatory
< provoostenator>
I'll do a more thorough comparison later.
< jnewbery>
wallet version is optional. Upgrading from an old version to new is optional
< meshcollider>
achow101: but why not just allow the wallet to be created with either version number
< jnewbery>
right
< ryanofsky>
jnewbery, i think version numbers are just confusing and should never be used again
< jnewbery>
They are confusing, but they at least cut down on the number of combinations of options
< ryanofsky>
your concern about testing and flag combinations is easily addressed by just refusing to load / support wallets with whatever combinations of flags you want to rule out
< jnewbery>
I think that's effectively the same thing, no?
< meshcollider>
IIRC in lightning, when exchanging feature lists, there are some optional features and some mandatory features based on bit positions
< ryanofsky>
yes, exactly
< achow101>
meshcollider: jnewbery: the problem with optional wallet versions is that any future wallet version becomes optional as well. You can't e.g. skip descriptor wallets and go to the version after that which introduces something completely unrelated to descriptors
< ryanofsky>
anything you want to do with version numbers is possible with flags, but flags are more readable and easier to think about
< achow101>
reconciling the two becomes a pain, as it was for hd wallets
< jnewbery>
achow101 that's always the case with version numbers
< meshcollider>
We dont have to add any new features to old style wallets from now on
< meshcollider>
Force a descriptor upgrade before any other upgrade
< ryanofsky>
because you don't have to mentally load the whole project history to figure out what conditions are possible, it's just written explicitly in code
< provoostenator>
I'd rather not force upgrades anytime soon. That most likely will lead to endless delays in shipping.
< jnewbery>
even if you don't make the version 'optional', there are still users of old wallets who would have to upgrade through descriptor wallets if they wanted a later version
< jnewbery>
provoostenator: definitely. No forced upgrades
< achow101>
i agree with ryanofsky
< achow101>
anyways, this is bikeshedding
< jnewbery>
?
< meshcollider>
I think this is important for the approach
< provoostenator>
What's more important I think is to decide if we want to support a hybrid with descriptors and regular stuff (I prefer not).
< achow101>
provoostenator: definitely no
< meshcollider>
No, we shouldnt
< provoostenator>
Ok, so that can be supported both with versioning and with feature flags I think. We can revisit that later?
< provoostenator>
Or is there something where this choice does matter more urgently?
< achow101>
meshcollider: jnewbery: consider the case where we introduce descriptor wallets as a new optional version. Sometime later down the road, we introduce a new wallet version because a new field is introduced in the wallet that is inherently not backwards compatible so it needs a version bump to prevent old software from loading, e.g. several more wallet flags
< meshcollider>
I think it just affects how to deal with transitioning tests
< achow101>
in order for users to upgrade to that version with more wallet flags, they first have to upgrade to descriptors, which they may not want to. that new version provides new functionality that does not require descriptors but is still applicable to old wallets. now this needs to be reconciled, and that's rather difficult
< ryanofsky>
yeah, i don't see why flags aren't just obviously better in every case. you can still prevent combinations of features, you are just forced to write down which combinations are possible
< jnewbery>
achow101 are you arguing that this should be a flag?
< meshcollider>
Flags alone dont prevent old nodes from trying to open though do they?
< achow101>
jnewbery: yes. if it is going to be an optional feature, it should be a flag
< ryanofsky>
meshcollider, mandatory flags do
< jnewbery>
Of course it's an optional feature. We don't force people to upgrade!
< meshcollider>
Ok yes I agree with making it a mandatory flag rather than version
< jnewbery>
I'm fine with it being a flag. Like ryanofsky has said, you can do everything with flags that you can with versions (and more)
< jnewbery>
As long as the valid combinations are all documented and enforced in one place
< provoostenator>
A mandatory flag also means we can reduce complexity.
< provoostenator>
Only supporting upgrades for basic cases.
< achow101>
alright, i'll change the pr to use a flag, restore the tests, and drop the upgrading logic for now
< meshcollider>
Just because its a flag doesn't mean we have to allow all possible combinations of flags
< meshcollider>
achow101: +1
< meshcollider>
Anything else for the last 10 minutes?
< meshcollider>
Anything else for backport to 0.18.0rc4 or high priority requests?
< jnewbery>
Wallet high priority PRs are #15006 #14447 #15741
< warren>
achow101: drawback of that is everyone's wallet.dat would be a different format, and you wouldn't be able to use the gitian binary
< gwillen>
this is a stopgap for using private-key-bearing descriptors in pre-descriptorwallets, but I expect that it can land MUCH faster than descriptorwallets will
< gwillen>
I expect that it is not eligible for backport, but it would be nice to see it in master
< jnewbery>
I'm fine with adding it. It does make the high priority list a bit wallet-heavy
< meshcollider>
The wallet is the only important thing anyway ;)
< gwillen>
hah :D
< luke-jr>
achow101: probably never?
< luke-jr>
achow101: modern bdb is AGPL; so we'd need to modify our software to serve source code of whatever is running to peers
< luke-jr>
which IMO is stupid
< bitcoin-git>
[bitcoin] jnewbery opened pull request #15810: [WIP] Remove nAbsurdFee fee from AcceptToMemoryPool (master...2019-04-remove-absurd-fee) https://github.com/bitcoin/bitcoin/pull/15810