Skip to content

Conversation

@PastaPastaPasta
Copy link
Member

@PastaPastaPasta PastaPastaPasta commented Apr 26, 2020

Backports 13028, 10740, 11851.
(see comment edit log to view seg fault that was occurring that has now been resolved)

@UdjinM6 UdjinM6 added the RPC Some notable changes to RPC params/behaviour/descriptions label Apr 27, 2020
@xdustinface
Copy link

and when using loadwallet from QT, a segmentation fault occurs

This helps here xdustinface@303df94

@UdjinM6
Copy link

UdjinM6 commented Apr 28, 2020

This requires many more backports for correct handling -nofoo cmd-line args/config options because of -nowallet - it doesn't work as expected atm.

@PastaPastaPasta
Copy link
Member Author

@UdjinM6 Can you point me to those PRs if you have any idea?

@UdjinM6
Copy link

UdjinM6 commented Apr 29, 2020

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.

@PastaPastaPasta
Copy link
Member Author

PastaPastaPasta commented Apr 30, 2020

cherry-picked, have not tested yet. Hopefully will test tonight. Looks like scripted diff fails. Usual

@PastaPastaPasta
Copy link
Member Author

PastaPastaPasta commented May 1, 2020

Mostly seems to work, but when using loadwallet in qt results in core dump

steps to reproduce: ./src/qt/dash-qt --testnet then open console and loadwallet wallet.dat

dash-qt: wallet/db.cpp:233: BerkeleyEnvironment::VerifyResult BerkeleyEnvironment::Verify(const string&, BerkeleyEnvironment::recoverFunc_type, std::__cxx11::string&): Assertion `mapFileUseCount.count(strFile) == 0' failed.
Posix Signal: Aborted
   0#: (0x557BC454A135) stl_vector.h:466    - std::vector<unsigned long, std::allocator<unsigned long> >::operator=(std::vector<unsigned long, std::allocator<unsigned long> >&&)
   1#: (0x557BC454A135) stacktraces.cpp:779 - HandlePosixSignal
   2#: (0x7FAD26A30890) <unknown-file>      - ???
   3#: (0x7FAD25B28E97) <unknown-file>      - ???
   4#: (0x7FAD25B2A801) <unknown-file>      - ???
   5#: (0x7FAD25B1A39A) <unknown-file>      - ???
   6#: (0x7FAD25B1A412) <unknown-file>      - ???
   7#: (0x557BC442F3A8) mutex:111           - std::recursive_mutex::lock()
   8#: (0x557BC442F3A8) sync.h:59           - AnnotatedMixin<std::recursive_mutex>::lock()
   9#: (0x557BC442F3A8) std_mutex.h:267     - std::unique_lock<CCriticalSection>::lock()
  10#: (0x557BC442F3A8) sync.h:128          - CCriticalBlock::Enter(char const*, char const*, int)
  11#: (0x557BC442F3A8) sync.h:149          - CCriticalBlock::CCriticalBlock(CCriticalSection&, char const*, char const*, int, bool)
  12#: (0x557BC442F3A8) db.cpp:232          - BerkeleyEnvironment::Verify(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, bool (*)(boost::filesystem::path const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&), std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&)
  13#: (0x557BC442F61D) db.cpp:350          - BerkeleyBatch::VerifyDatabaseFile(boost::filesystem::path const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, bool (*)(boost::filesystem::path const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&))
  14#: (0x557BC43D43FC) wallet.cpp:5062     - CWallet::Verify(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&)
  15#: (0x557BC4395F90) basic_string.h:211  - std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_is_local() const
  16#: (0x557BC4395F90) basic_string.h:220  - std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_dispose()
  17#: (0x557BC4395F90) basic_string.h:647  - std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::~basic_string()
  18#: (0x557BC4395F90) rpcwallet.cpp:2911  - loadwallet(JSONRPCRequest const&)
  19#: (0x557BC41BA26D) server.cpp:565      - CRPCTable::execute(JSONRPCRequest const&) const
  20#: (0x557BC3E10F0A) univalue.h:20       - UniValue::operator=(UniValue&&)
  21#: (0x557BC3E10F0A) rpcconsole.cpp:323  - RPCConsole::RPCParseCommandLine(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, bool, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*)
  22#: (0x557BC3E117A3) rpcconsole.cpp:429  - RPCExecutor::request(QString const&)
  23#: (0x557BC3E11F69) rpcconsole.moc:78   - RPCExecutor::qt_static_metacall(QObject*, QMetaObject::Call, int, void**)
  24#: (0x557BC5169FF2) <unknown-file>      - ???
Aborted (core dumped)

@xdustinface
Copy link

xdustinface commented May 3, 2020

Mostly seems to work, but when using loadwallet in qt results in core dump

c7e6adf86963c62a146857d8958cc9b943d3bbd0 This one fixes the crash.. Happened because the wallet object had no name set by default. This led to not keeping track of wallet.dat as used file and then to this failing assertion assert(mapFileUseCount.count(strFile) == 0).

Also feel free to apply those two if you want

  1. 9d811e8e39d5621b2de1b3d58925c615519cdfb9 Use a constant for the default name in all other places of the code

  2. e412f598def029b213af23a42a368b1882c6d139 and to fully avoid this crash throw an error if someone starts with -wallet=""

@PastaPastaPasta
Copy link
Member Author

PastaPastaPasta commented May 6, 2020

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

@UdjinM6
Copy link

UdjinM6 commented May 9, 2020

Going back to default file name is not a good idea imo, because -wallet was redefined to specify a path, not a file. But crashing is not good either, would prefer at least some meaningful response back to the user, smth like e412f59 proposed above but maybe saying "-wallet can't be an empty string" or smth like that (i.e. mentioning the exact setting that caused an error and not mentioning "file" cause it's not a file anymore).

laanwj and others added 3 commits May 9, 2020 17:35
…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
@PastaPastaPasta
Copy link
Member Author

@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.

@PastaPastaPasta PastaPastaPasta added this to the 16 milestone May 9, 2020
@PastaPastaPasta
Copy link
Member Author

PastaPastaPasta commented May 10, 2020

force pushed doing two things, one, based this branch on #3472, two fixed scripted diff, also "rebased" on latest develop in the process

@PastaPastaPasta
Copy link
Member Author

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.

@PastaPastaPasta
Copy link
Member Author

PastaPastaPasta commented May 10, 2020

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

@PastaPastaPasta PastaPastaPasta requested a review from UdjinM6 May 10, 2020 06:38
Copy link

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.

Comment on lines -288 to -293
Copy link

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

laanwj added 2 commits May 10, 2020 11:15
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
@PastaPastaPasta
Copy link
Member Author

see latest, removed release-note doc and squashed, and cherry-picked be1253e996ae44dd56e437a6fdf3d8abbbeced8f

@PastaPastaPasta PastaPastaPasta requested a review from UdjinM6 May 10, 2020 16:17
xdustinface
xdustinface previously approved these changes May 10, 2020
Copy link

@xdustinface xdustinface left a 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

jnewbery and others added 7 commits May 10, 2020 12:13
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>
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

RPC Some notable changes to RPC params/behaviour/descriptions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants