-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Backport loadwallet #3462
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Backport loadwallet #3462
Conversation
This helps here xdustinface@303df94 |
|
This requires many more backports for correct handling |
|
@UdjinM6 Can you point me to those PRs if you have any idea? |
b1f1629 to
adc2353
Compare
|
Pls see https://github.com/UdjinM6/dash/commits/pr3462. It's really bitcoin#12713 + 4d34fcc from bitcoin#11862 that are needed but had to backport other PRs to avoid (large) merge conflicts. Might backport a couple of first commits instead of a full 11862 if network specific config sections is not smth we want (right now). Pls note, I did NOT yet throughly self-review/test these, just made sure changes somewhat made sense when resolving (trivial-ish) merge conflicts and verified that it compiles and integration tests pass locally. |
|
cherry-picked, have not tested yet. Hopefully will test tonight. Looks like scripted diff fails. Usual |
|
Mostly seems to work, but when using loadwallet in qt results in core dump steps to reproduce: |
c7e6adf86963c62a146857d8958cc9b943d3bbd0 This one fixes the crash.. Also feel free to apply those two if you want
|
|
According to @xdustinface, the crash that I pointed out actually occurs in bitcoin too(at least in 0.17), maybe this is an okay crash to have rn? Thoughts @UdjinM6 |
|
Going back to default file name is not a good idea imo, because |
b04282f to
9802c6b
Compare
…path mangling 5460460 Add AbsPathForConfigVal to consolidate datadir prefixing for path args (James O'Beirne) a1e1305 Clarify help messages for path args to mention datadir prefix (James O'Beirne) Pull request description: Change `-conf`'s help message to indicate that relative path values will be prefixed by the datadir path. This behavior probably merits clarification; it's kind of confusing when attempting to specify a configuration file in the current directory with `-conf=bitcoin.conf`, but instead loading the `bitcoin.conf` file in ~/.bitcoin datadir. ### Edit This PR has been modified to document all cases where relative path configurations are modified to be under datadir. A small refactoring has also been added which consolidates this normalization. Tree-SHA512: be4fc0595fbeba33d17af08f59898af45e76a44f00719ea0282403b155ac6755584604fab765250a3aa14ed6991882c4d1ccbe601184362c5ba97c886bdda344
f7683cb Track negated arguments in the argument paser. (Evan Klitzke) 4f872b2 Add additional tests for GetBoolArg() (Evan Klitzke) Pull request description: This change explicitly enable tracking negated options in the option parser. A negated option is one passed with a `-no` prefix. For example, `-nofoo` is the negated form of `-foo`. Negated options were originally added in the 0.6 release. The change here allows code to explicitly distinguish between cases like `-nofoo` and `-foo=0`, which was not possible previously. The option parser does not have any changed semantics as a result of this change, and existing code will parse options just as it did before. The motivation for this change is to provide a way to disable options that are otherwise not boolean options. For example, the `-debuglogfile` option is normally interpreted as a string, where the value is the log file name. With this change a user can pass in `-nodebuglogfile` and the code can see that it was explicitly negated, and use that to disable the log file. This change originally split out from bitcoin#12689. Tree-SHA512: cd5a7354eb03d2d402863c7b69e512cad382781d9b8f18c1ab104fc46d45a712530818d665203082da39572c8a42313c5be09306dc2a7227cdedb20ef7314823
…ation for network-specific sections 77a733a [tests] Add additional unit tests for -nofoo edge cases (Anthony Towns) af173c2 [tests] Check GetChainName works with config entries (Anthony Towns) fa27f1c [tests] Add unit tests for ReadConfigStream (Anthony Towns) 087c5d2 ReadConfigStream: assume the stream is good (Anthony Towns) 6d5815a Separate out ReadConfigStream from ReadConfigFile (Anthony Towns) 834d303 [tests] Add unit tests for GetChainName (Anthony Towns) 11b6b5b Move ChainNameFromCommandLine into ArgsManager and rename to GetChainName (Anthony Towns) Pull request description: This does a bit of refactoring of the configuration handling code in order to add additional tests to make adding support for [test]/[regtest] sections in the config file in bitcoin#11862 easier. Should not cause any behaviour changes. Tree-SHA512: 8d2ce1449fc180de03414e7e569d1a21ba1e9f6564e13d3faf3961f710adc725fa0d4ab49b89ebd2baa11ea36ac5018377f693a84037d386a8b8697c9d6db3e9
|
@UdjinM6 @xdustinface Regarding this particular crash, I think it's okay for now, bitcoin had it bitcoin#14538 fixed by bitcoin#14552 however 14552 is locked behind some further backporting. After this is merged I might be able to backport the required stuff to fix this bug in 0.16, if we don't have enough time we just note it as a known issue in release notes. |
9802c6b to
89455f6
Compare
|
force pushed doing two things, one, based this branch on #3472, two fixed scripted diff, also "rebased" on latest develop in the process |
|
In testing, everything seems to work, I'm able to load a directory based wallet, and I'm able to load a file based wallet. Everything seems dandy to me. Ofc there is that one crash when you are using wallet="" and try to load wallet.dat, but as I pointed out above, I think that is fine for now. |
89455f6 to
ee44662
Compare
|
squashed all the 10740 changes into one commit to make this PR nicer, can now be merged w/o squashing and w/o making a mess of the git history |
doc/release-notes-pr12823.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should either keep one from 10740 or drop this file too for consistency sake.
src/wallet/init.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Aaaaand it's gone"? Pls see be1253e996
c25321f Add config changes to release notes (Anthony Towns) 5e3cbe0 [tests] Unit tests for -testnet/-regtest in [test]/[regtest] sections (Anthony Towns) 005ad26 ArgsManager: special handling for -regtest and -testnet (Anthony Towns) 608415d [tests] Unit tests for network-specific config entries (Anthony Towns) 68797e2 ArgsManager: Warn when ignoring network-specific config setting (Anthony Towns) d1fc4d9 ArgsManager: limit some options to only apply on mainnet when in default section (Anthony Towns) 8a9817d [tests] Use regtest section in functional tests configs (Anthony Towns) 30f9407 [tests] Unit tests for config file sections (Anthony Towns) 95eb66d ArgsManager: support config file sections (Anthony Towns) 4d34fcc ArgsManager: drop m_negated_args (Anthony Towns) 3673ca3 ArgsManager: keep command line and config file arguments separate (Anthony Towns) Pull request description: The weekly meeting on [2017-12-07](http://www.erisian.com.au/meetbot/bitcoin-core-dev/2017/bitcoin-core-dev.2017-12-07-19.00.log.html) discussed allowing options to bitcoin to have some sensitivity to what network is in use. @theuni suggested having sections in the config file: <cfields> an alternative to that would be sections in a config file. and on the cmdline they'd look like namespaces. so, [testnet] port=5. or -testnet::port=5. This approach is (more or less) supported by `boost::program_options::detail::config_file_iterator` -- when it sees a `[testnet]` section with `port=5`, it will treat that the same as "testnet.port=5". So `[testnet] port=5` (or `testnet.port=5` without the section header) in bitcoin.conf and `-testnet.port=5` on the command line. The other aspect to this question is possibly limiting some options so that there is no possibility of accidental cross-contamination across networks. For example, if you're using a particular wallet.dat on mainnet, you may not want to accidentally use the same wallet on testnet and risk reusing keys. I've set this up so that the `-addnode` and `-wallet` options are `NETWORK_ONLY`, so that if you have a bitcoin.conf: wallet=/secret/wallet.dat upnp=1 and you run `bitcoind -testnet` or `bitcoind -regtest`, then the `wallet=` setting will be ignored, and should behave as if your bitcoin.conf had specified: upnp=1 [main] wallet=/secret/wallet.dat For any `NETWORK_ONLY` options, if you're using `-testnet` or `-regtest`, you'll have to add the prefix to any command line options. This was necessary for `multiwallet.py` for instance. I've left the "default" options as taking precedence over network specific ones, which might be backwards. So if you have: maxmempool=200 [regtest] maxmempool=100 your maxmempool will still be 200 on regtest. The advantage of doing it this way is that if you have `[regtest] maxmempool=100` in bitcoin.conf, and then say `bitcoind -regtest -maxmempool=200`, the same result is probably in line with what you expect... The other thing to note is that I'm using the chain names from `chainparamsbase.cpp` / `ChainNameFromCommandLine`, so the sections are `[main]`, `[test]` and `[regtest]`; not `[mainnet]` or `[testnet]` as might be expected. Thoughts? Ping @meshcollider @laanwj @jonasschnelli @morcos Tree-SHA512: f00b5eb75f006189987e5c15e154a42b66ee251777768c1e185d764279070fcb7c41947d8794092b912a03d985843c82e5189871416995436a6260520fb7a4db
e2f58f4 wallet: Make vpwallets usage thread safe (João Barbosa) Pull request description: This PR turns the functions introduced in bitcoin#13017 thread safe. This is required to correctly support dynamically loading wallets, which is implemented in bitcoin#10740. Tree-SHA512: efaa09e501636cf957aa33de83719ce09dc0c2a19daff741a94ef10d6b7ba5dee538355b80c96ead995140f99f5df0c92fb0e22ae1adb8f397eb478280c8d8c7
ee44662 to
ecd4109
Compare
|
see latest, removed release-note doc and squashed, and cherry-picked be1253e996ae44dd56e437a6fdf3d8abbbeced8f |
xdustinface
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't find any further issue in the code besides the small indentation thing.
- Loading wallets works besides the known issues.
- Loaded wallets appear to work as expected too.
ACK
cd53981 [docs] Add release notes for `loadwallet` RPC. (John Newbery) a46aeb6 [wallet] [tests] Test loadwallet (John Newbery) 5d15260 [wallet] [rpc] Add loadwallet RPC (John Newbery) 876eb64 [wallet] Pass error message back from CWallet::Verify() (John Newbery) e0e90db [wallet] Add CWallet::Verify function (John Newbery) 470316c [wallet] setup wallet background flushing in WalletInit directly (John Newbery) 59b87a2 [wallet] Fix potential memory leak in CreateWalletFromFile (John Newbery) Pull request description: Adds a `loadwallet` RPCs. This allows wallets to be loaded dynamically during runtime without having to stop-start the node with new `-wallet` params. Includes functional tests and release notes. Limitations: - currently this functionality is only available through the RPC interface. - wallets loaded in this way will not be displayed in the GUI. Tree-SHA512: f80dfe32b77f5c97ea3732ac538de7d6ed7e7cd0413c2ec91096bb652ad9bccf05d847ddbe81e7cd3cd44eb8030a51a5f00083871228b1b9b0b8398994f6f9f1
-BEGIN VERIFY SCRIPT-
sed -i 's/\<CWalletDBWrapper\>/BerkeleyDatabase/g' src/wallet/db.h src/wallet/db.cpp
sed -i '/statuses/i/** Backend-agnostic database type. */\nusing WalletDatabase = BerkeleyDatabase\;\n' src/wallet/walletdb.h
ren() { git grep -l "\<$1\>" 'src/*.cpp' 'src/*.h' ':(exclude)*dbwrapper*' test | xargs sed -i "s:\<$1\>:$2:g"; }
ren CDBEnv BerkeleyEnvironment
ren CDB BerkeleyBatch
ren CWalletDBWrapper WalletDatabase
ren CWalletDB WalletBatch
ren dbw database
ren m_dbw m_database
ren walletdb batch
ren pwalletdb batch
ren pwalletdbIn batch_in
ren wallet/batch.h wallet/walletdb.h
ren pwalletdbEncryption encrypted_batch
-END VERIFY SCRIPT-
Signed-off-by: Pasta <pasta@dashboost.org>
Text from bitcoin#11851 (comment) by John Newbery <john@johnnewbery.com>.
Due to not disconnecting the signal there the trace of the `loadwallet` rpc command ends in a call of `SplashScreen::ConnectWallet` for the not longer existent splashscreen object. https://github.com/dashpay/dash/blob/7770a7da959dfd4c06c6e4b32fb70c0c7bb59085/src/wallet/wallet.cpp#L4192 Wasn't an issue before the `loadwallet` rpc was introduced because `CWallet::LoadWallet` was only called once on startup.
Co-Authored-By: dustinface <35775977+xdustinface@users.noreply.github.com>
152f78a to
ed7d8c8
Compare
UdjinM6
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
Backports 13028, 10740, 11851.
(see comment edit log to view seg fault that was occurring that has now been resolved)