0481fa2584 util: refactor upper/lowercase functions (Karl-Johan Alm)
Pull request description:
This includes renaming Downcase() to ToLower() and make it return a string rather than modify referenced arg.
Also adds ToUpper() string version.
Additionally, it clarifies that the locale independency of the case functions is a *feature* and not a limitation. I interpreted it as the latter and rewrote code to be locale-aware before realizing this.
This is done in preparation for #11413 and as a general refactor. I don't think the optimization that the pre-refactor state gave warrants the unwieldy usage.
ACKs for top commit:
laanwj:
ACK 0481fa2584 from me
practicalswift:
ACK 0481fa2584 -- diff looks correct
l2a5b1:
ACK 0481fa2 - Although, I think @luke-jr's [feedback](https://github.com/bitcoin/bitcoin/pull/16566#issuecomment-519580760) is spot on; `Downcase` is just an artifact of `ParseNetwork`, which has been happily downcasing `net` via a string argument for over 7 years; and I do recommend to add `ToLower` *when* somebody actually needs it in new work, there is no point in keeping a trivial utility function if it is not appreciated.
promag:
ACK 0481fa2584.
Tree-SHA512: 9b834ecc1b97db043e261bcbc59e42372e11e2fb9a6943688f18a835bf5c9205f68e4614f58e90ba260d1b8f0e060c6f67b390b62436c21b56891db23bc41628
6285a318d7 Remove redundant WalletController::addWallet slot (Hennadii Stepanov)
Pull request description:
~~Fix #15453.~~ It is fixed by https://github.com/bitcoin/bitcoin/pull/16348#issuecomment-509308347
The _only_ reason of these lines on master (8c69fae944)
2679bb8919/src/qt/walletcontroller.cpp (L121-L128)
is to `Q_EMIT walletAdded(wallet_model);` in a thread-safe manner;
This PR makes this in a line of code:
1b83875006/src/qt/walletcontroller.cpp (L121)
EDITED:
To establish the ownership of a new `WalletModel` object is not necessary on the master (https://github.com/bitcoin/bitcoin/pull/16349#discussion_r301679192 by **promag**).
But:
> it's good habit to set ownership
And I agree. It is a safe practice.
ACKs for top commit:
promag:
ACK 6285a318d7.
jonasschnelli:
utACK 6285a318d7
ryanofsky:
utACK 6285a318d7. Only change since last review is rebasing and restoring a deleted comment. I do think the comments I suggested last review would be better than this one, but this is at least better than before.
Tree-SHA512: 90370cb1fe853b84dd16c3781ba4f97f3f4deca56bba0203e457f37b3220fd13228cf8495fd882ff18b7c782c27544cc2e7a88aaec5b69b9ef6d8626bdaaf332
1f26328c1b doc: Update Markdown syntax for bdb packages (Emil Engler)
Pull request description:
This pull requests adds single back quotes to libdb-dev and libdb++-dev.
The reason for this is that I (and probably others) overlook them too often and they look like normal text because they have no style.
ACKs for top commit:
fanquake:
ACK 1f26328c1b
Tree-SHA512: cd52e34f5b063f9d4337e1fcb04459d2c40be47afe5118c27d949a521980413aefe3ecb862b7b427e576c7690b999f7abdd53193e1b110c3f64a4cd1c3a41cbc
bf72b8a555 build: disable libsecp256k1 benchmarks (fanquake)
Pull request description:
These were previously disabled, but upstream changed to having benchmarks enabled by default
in https://github.com/bitcoin-core/secp256k1/pull/480 and we pulled that change in as part of #15703.
ACKs for top commit:
laanwj:
ACK bf72b8a555
real-or-random:
ACK bf72b8a555 I only looked at the diff
practicalswift:
ACK bf72b8a555 -- diff looks correct
Tree-SHA512: f4c99c774c8bfd37f98fa200667530988ef8da61920fafdff7e929d9dc5dd8304981b625611f6c3fbc525172d269a2a1d33e592297bd8ff418dff11b05b5e204
Restores the confirmed/conflicted tx check removed in
8753f5652b. There should be no external
behaviour change (these txs would not get accepted to the mempool
anyway), but not having the check in the wallet causes log spam.
Also adds a comment to ResentWalletTransactions() that
confirmed/conflicted tx check is done in SubmitMemoryPoolAndRelay().
b7b9f6e4ce Remove p2pEnabled from Chain interface (Antoine Riard)
Pull request description:
RPC server starts in warmup mode, it can't process yet calls, then follows connection manager initialization and finally RPC server get out of warmup mode. RPC calls shouldn't be able to get P2P disabled errors because once we initialize g_connman it's not unset until shutdown, after RPC server has been stopped.
@mzumsande comment in #15713 let me thought that `p2pEnabled` was maybe useless, `g_connman` is always initialized before RPC server is getting out of warmup. These checks against P2P state were introduced in 5b446dd5b1.
ACKs for top commit:
promag:
ACK b7b9f6e4ce
jnewbery:
ACK b7b9f6e4ce
Tree-SHA512: 4de2b9fc496bf8347ff5cc645848a5a44c8ca7596cd134f17f3088f5f8262d1d88b8e2a052df93e309ec9a81956a808df17a9eb9f10d4f4d693c95d607fe3561
61c5c64b31 doc: Add historical release notes for 0.18.1 (Wladimir J. van der Laan)
Pull request description:
ACKs for top commit:
fanquake:
ACK 61c5c64b31
Tree-SHA512: d98011ee79ae9fb69bd4c0d408b87fd05c640e35683b44bacbb49dbc7e66ebb754f46edd268af611b3fd1c171c2a723d516a21099bd6fd015491f0f4119ca33e
RPC server starts in warmup mode, it can't
process yet calls, then follows connection manager
initialization and finally RPC server get out of
warmup mode. RPC calls shouldn't be able to get
P2P disabled errors because once we initialize
g_connman it's not unset until shutdown, after
RPC server has been stopped.
2f37163caf test: only include and init openSSL where it's actually used (fanquake)
Pull request description:
This should save initializing OpenSSL during tests if compiled with `--disable-bip70`.
ACKs for top commit:
laanwj:
straightforward ACK 2f37163caf
Tree-SHA512: 513e867718c79b014b4d470404c891d83c21df92eed07abaccd89b88a32f8b801e402e7d0d400c49c1b964f4b76409ca17fb90bde2e2c45dcf8a1897b452683f
fa76285fdd test: Explain why -whitelist is used in feature_fee_estimation (MarcoFalke)
faff85a69a test: Format feature_fee_estimation with pep8 (MarcoFalke)
Pull request description:
ACKs for top commit:
practicalswift:
ACK fa76285fdd -- diff looks correct
Sjors:
ACK fa76285, every bit of clarification helps. It's clear that without `-whitelist` the test becomes extremely slow (it does pass).
Tree-SHA512: 13ec7e4cd0409e7bb76cbcd344e31c0f612c8ce4a1f1ec6ceaedf345f634bc09786ed38d38920c3469b2862c856ee3e5e42534ef90f531bd8dc83c3db3c06417
b2ea20d330 doc: Fix grammar and punctuation in developer notes (Kristian Kramer)
Pull request description:
This pull request is regarding minor grammar and punctuation errors in the developer notes. There were no modifications to the existing code, only alterations to fix the grammar and punctuation in the text to make the developer notes more understandable and easier to read.
ACKs for top commit:
fanquake:
ACK b2ea20d330
Tree-SHA512: eef990b7e7645b44a1ab0b057f4df35894c307fd23cc861a10d3cc80e7fe7fe8f5b94467f8224cc8a7aaa226f82be3a1f0460a45f3e25e5dab1e1d333c9edbc0
fa8489a155 test: Add test for BIP30 duplicate tx (MarcoFalke)
77770d95e2 test: Properly serialize BIP34 coinbase height (MarcoFalke)
Pull request description:
This adds a test for BIP30 to check that duplicate txs can exist in the blockchain given the first one was completely spent when the second one is added. (Requested by ajtowns in https://github.com/bitcoin/bitcoin/pull/16333#issuecomment-508604071)
We can not add a test that a later duplicate tx overwrites a previous one, because BIP30 is always enforced on regtest. If someone feels strongly about such a test, some Bitcoin Core code would have to be modified, which can be done in a follow up pull request.
Also, add a commit to fix the BIP34 test failures reported in https://github.com/bitcoin/bitcoin/pull/14633#issue-227712540
ACKs for top commit:
laanwj:
Code review ACK fa8489a155
Tree-SHA512: c707d0bdc93937263876b603425b53322a2a9f9ec3f50716ae2fa9de8ddc644beb22b26c1bfde7f4aab102633e096b354ef380db919176bd2cb44a2828f884aa
faf36838bd test: Avoid hardcoding the chain name in combine_logs (MarcoFalke)
fa8a1d7ba3 test: Adapt test framework for chains other than "regtest" (MarcoFalke)
68f546635d test: Fix “local variable 'e' is assigned to but never used” (Ben Woosley)
Pull request description:
This is required for various work in progress:
* testchains #8994
* signet #16411
* some of my locally written tests
While it will be unused in the master branch as of now, it will make all of those pull requests shorter. Thus review for non-regtest tests can focus on the actual changes and not some test framework changes.
ACKs for top commit:
jonatack:
ACK faf36838bd, ran tests and reviewed the code.
Tree-SHA512: 35add66c12cab68f2fac8f7c7d47c604d3f24eae9336ff78f83e2c92b3dc08a25e7f4217199bac5393dd3fb72f945bba9c001d6fbb8efd298c88858075fcb3d6
fab3658356 [qa] Test that getdata requests work as expected (Suhas Daftuar)
fa883ab35a net: Use mockable time for tx download (MarcoFalke)
Pull request description:
Two commits:
* First commit changes to mockable time for tx download (refactoring, should only have an effect on regtest)
* Second commit adds a test that uses mocktime to test tx download
ACKs for top commit:
laanwj:
code review ACK 16197/commits/fab365835639a3da03f8ad9a58a0db6c6c4c2314
jamesob:
ACK fab3658356
Tree-SHA512: 3a64a3e283ec4bab1f6e506404b11f0a564a5b61d2a7508ae738a61f035e57220484c66e0ae47d847fe9f7e3ff5cc834909d7b34a9bbcea6abe01f8742806908
05fdb97df4 [doc] Update and extend benchmarking.md (Antoine Riard)
Pull request description:
Trying to make benchmarking docs a bit more friendly.
If you have any more ideas, specially on the Notes section, which component need more benchmarks.
(oh isn't a write-up somewhere to generate flame graphs for core ?)
ACKs for top commit:
jonatack:
ACK 05fdb97df4
fanquake:
ACK 05fdb97df4 - with the single nit.
Tree-SHA512: 1d31438065cab12b43b0227c1c774b412ac3d9d46d4cbe69cfe753424a81e51839777e815c70880da8ae6c8fb95221dc7559334eeb550221e8a76fb20a370f75
fa566b2601 test: Add missing sync_blocks to feature_pruning (MarcoFalke)
Pull request description:
Fixes#16537Fixes#16520
ACKs for top commit:
promag:
ACK fa566b2601.
jonatack:
ACK fa566b2601. These past few months I have been seeing intermittent failures with this test on master. Ran `(for i in {1..40}; do test/functional/feature_pruning.py -l=debug; done)` overnight with this change; no failures.
Tree-SHA512: 5181d5ea525f43ad09e1c8b9ae72e32219f483948854c6dc07dda24b790cbdf4012e586253a0e158a71a980d1ca9f5fdf06aafbe95b8ea3d9154ef2c8687395b
0a4e4a05ea [build] .gitignore add Qt Creator Makefile.am.user (Peter Bushnell)
Pull request description:
Opening Bitcoin with Qt Creator via the Makefile.am generates a Makefile.am.user. Would be handy to have this file ignored. Looking around I can see this file has snuck into a few downstream projects. I do personally find myself editing commits to remove this file when I've not been paying attention. There's got to have been a few PRs with this file accidentally attached.
ACKs for top commit:
promag:
ACK 0a4e4a05ea.
practicalswift:
ACK 0a4e4a05ea -- diff looks correct
fanquake:
ACK 0a4e4a05ea - Something like this might be better in your local .gitignore_global, but we do have a section on Qt Creator in the docs, so I guess this is fine.
Tree-SHA512: 282fd37ec6b313bf86ebe85a42e7f05b14969c683d32f01d2561f4e15d90a2e89f4d0e3c227cb3350684bcae21d178ea343edb32c4c1d33f4849246222c9b8d5
This pull request is regarding minor grammar and punctuation errors in the developer notes. There were no modifications to the existing code, only alterations to fix the grammar and punctuation in the text.
Update developer-notes.md
Update developer-notes.md
Revert "Update developer-notes.md"
This reverts commit dfeb0bacb054ed24766f8af7bae0c0166b0cb4cb.
e6f649cb2c test: Make tests arg type specific (Hennadii Stepanov)
b70cc5d733 Revamp option negating policy (Hennadii Stepanov)
db08edb303 Replace IsArgKnown() with FlagsOfKnownArg() (Hennadii Stepanov)
dde80c272a Use ArgsManager::NETWORK_ONLY flag (Hennadii Stepanov)
9a12733508 Remove unused m_debug_only member from Arg struct (Hennadii Stepanov)
fb4b9f9e3b scripted-diff: Use ArgsManager::DEBUG_ONLY flag (Hennadii Stepanov)
1b4b9422ca scripted-diff: Use Flags enum in AddArg() (Hennadii Stepanov)
265c1b58d8 Add Flags enum to ArgsManager (Hennadii Stepanov)
e0d187dfeb Refactor InterpretNegatedOption() function (Hennadii Stepanov)
e0e18a1017 refactoring: Check IsArgKnown() early (Hennadii Stepanov)
Pull request description:
This PR adds the `Flags` enum to the `ArgsManager` class. Also the `m_flags` member is added to the `Arg` struct. Flags denote an allowed type of an arg value and special hints.
This PR is only a refactoring and does not change behavior.
ACKs for top commit:
jamesob:
ACK e6f649cb2c
MarcoFalke:
ACK e6f649cb2c thanks for adding types to the command line options
Tree-SHA512: b867f8a9cbce2d2473c293d534af662d8cd5be15060ff0682e97af678974bdaac35e8bc6328ccba32f105034bcd38f169b92a6fb67798667891ce14d5d2a2dea
fb62f128bb Tidy up BroadcastTransaction() (John Newbery)
b8eecf8e79 Remove unused submitToMemoryPool and relayTransactions Chain interfaces (Antoine Riard)
8753f5652b Remove duplicate checks in SubmitMemoryPoolAndRelay (Antoine Riard)
611291c198 Introduce CWalletTx::SubmitMemoryPoolAndRelay (Antoine Riard)
8c8aa19b4b Add BroadcastTransaction utility usage in Chain interface (Antoine Riard)
Pull request description:
Remove CWalletTx::AcceptToMemoryPool
Replace CWalletTx::RelayWalletTransaction by SubmitMemoryPoolAndRelay
Add a relay flag to broadcastTransaction because wasn't sure of ReacceptWalletTransactions semantic.
Obviously, working on implementing https://github.com/bitcoin/bitcoin/pull/14978#issuecomment-459373984 to add the new higher-method in Node interface, will add a commit, just need more thought to do it cleanly
ACKs for top commit:
MarcoFalke:
re-ACK fb62f128bb
Sjors:
re-ACK fb62f128bb
Tree-SHA512: a7ee48b0545f537fa65cac8ed4cb24e777ab90b877d4eefb87971fa93c6a59bd555b62ad8940c6ffb40592a0bd50787d27587af99f20b56af72b415b6394251f
d6b3640ac7 [test] walletcreatefundedpsbt: check RBF is disabled when -walletrbf=0 (Sjors Provoost)
9ed062b568 [doc] rpc: remove "fallback to" from RBF default help (Sjors Provoost)
4fcb698bc2 [rpc] walletcreatefundedpsbt: use wallet default RBF (Sjors Provoost)
Pull request description:
The `walletcreatefundedpsbt` RPC call currently ignores `-walletrbf` and defaults to not use RBF. This PR fixes that.
This PR also replaces UniValue in `ConstructTransaction` with a `bool` in preparation of moving this helper method out of the RPC codebase entirely. This may be a bit overkill, but does slightly simplify it.
Fixes#15878
ACKs for top commit:
achow101:
Code Review ACK d6b3640ac7
l2a5b1:
re-ACK d6b3640
MarcoFalke:
ACK d6b3640ac7
Tree-SHA512: 55b9bccd1ef36b54f6b34793017dc0721103099ad3761b3b04862291ee13d6915915d4dbb1a8567924fa56e5e95dfe10eec070e06701610e70c87f8ea92b2a00
Opening Bitcoin with Qt Creator via the Makefile.am generates a Makefile.am.user. Would be handy to have this file ignored. Looking around I can see this file has snuck into a few downstream projects. I do personally find myself editing commits to remove this file when I've not been paying attention. There's got to have been a few PRs with this file accidentally attached.
IsCoinBase check is already performed early by
AcceptToMemoryPoolWorker
GetDepthInMainChain check is already perfomed by
BroadcastTransaction
To avoid deadlock we MUST keep lock order in
ResendWalletTransactions and CommitTransaction,
even if we lock cs_main again further.
in BroadcastTransaction. Lock order will need
to be clean at once in a future refactoring
Access through a broadcastTransaction method.
Add a wait_callback flag to turn off race protection when wallet
already track its tx being in mempool
Standardise highfee, absurdfee variable name to max_tx_fee
We drop the P2P check in BroadcastTransaction as g_connman is only
called by RPCs and the wallet scheduler, both of which are initialized
after g_connman is assigned and stopped before g_connman is reset.
7a0c224289 Suppress output in test_bitcoin for expected errors (Gert-Jaap Glasbergen)
Pull request description:
Closes#15944
This adds two methods to noui, that allows temporarily suppressing (and then resuming) the output from `noui`. For situations where errors are expected, it's confusing for the test binary to output an error and then conclude with `No errors detected`.
It also uses this supress/reconnect in the tests that currently produce verbose errors when running `test_bitcoin`.
Output of `test_bitcoin` on current master:
```
gertjaap@gjdesktop:~/src/bitcoin$ src/test/test_bitcoin
Running 351 test cases...
Error: Specified -walletdir "/tmp/test_common_Bitcoin Core/1561389554_943311758/tempdir/path_does_not_exist" does not exist
Error: Specified -walletdir "/tmp/test_common_Bitcoin Core/1561389554_643733972/tempdir/not_a_directory.dat" is not a directory
Error: Specified -walletdir "wallets" is a relative path
*** No errors detected
```
Output after this code is merged:
```
gertjaap@gjdesktop:~/src/bitcoin$ src/test/test_bitcoin
Running 351 test cases...
*** No errors detected
```
ACKs for top commit:
l2a5b1:
ACK 7a0c224 - tested and reviewed.
laanwj:
ACK 7a0c224289
Tree-SHA512: c7881f7a431a065329360ffa9937ce4742694c646c90c019d3aff95dfd7fccbdcda9116c5762feb6dfd1108d14f9fb386e203b173c4bde9093afb2b8c977d13d
fac2e6a604 test: Fail early on disconnect in mininode.wait_for_* (MarcoFalke)
Pull request description:
The node might crash or disconnect when our mininode waits for data. Due to the crash, the data is guaranteed to never arrive and we can fail early with an assert
ACKs for top commit:
laanwj:
ACK fac2e6a604
Tree-SHA512: 32ca844eb66bd70ea49103d51c76b953242b1886e0834d96fca8840fc984ff40346d0a799adf8f76b03514a783cb9cec69d45e00bdd328c5192c31b5d8d17af2
b078067b9c gui: Remove unused RPCConsole::tabFocus (João Barbosa)
Pull request description:
Added in #14573 but not used, so begone.
ACKs for top commit:
practicalswift:
utACK b078067b9c
hebasto:
ACK b078067b9c
laanwj:
ACK b078067b9c, there's nothing really to test here
Tree-SHA512: 237276dea4d174b5fca34855447146f79c3faaae7179f4245c70e2070b49282d95f886b1be6d2a33713c81a254f4483a4e4bf850053a8dcb18a3a897bd3da08e
c5d3787367 Allow createwallet to take empty passwords to make unencrypted wallets (Andrew Chow)
Pull request description:
Allow createwallet to take the empty string as a password and interpret that as leaving the wallet unencrypted. Also warn when that happens.
This fixes a bug where it was not possible to use the `avoid_reuse` option for new unencrypted wallets without using named arguments.Thus this allows more `createwallet` options to be added that can be set on unencrypted wallets when using positional arguments.
ACKs for top commit:
jnewbery:
code review ACK c5d3787367
meshcollider:
re-utACK c5d3787367
ryanofsky:
utACK c5d3787367. Changes since last review are rebasing, concatenating warning strings to avoid discarding warnings, adding release notes, and choosing an unambiguous wallet name for the test.
Tree-SHA512: 146737a728dd614ba94d4b166b27e8c9e195badd1709ccab2315afe59176d9b493dfba9b61c3ed81090f059c7e464d709deb06d99451b9a3fff667f527d6f7c9
0b1f4b3c66 wallet: Drop unused OldKey (João Barbosa)
Pull request description:
Replaces #16494, `OldKey` (previously `CWalletKey`) was never serialized in the code history which means that unserialization support is not required, so remove the code entirely.
ACKs for top commit:
jnewbery:
ACK 0b1f4b3c66
laanwj:
ACK 0b1f4b3c66
fanquake:
ACK 0b1f4b3c66
Tree-SHA512: 92e9b2d6fc41f2765492d5d69d18fc4302c40ab44f28c8c30ca652c72767fbc484848c51a38ecf1f447849767a583c398784408bb5f64f9c86f9a5872b325ffc
fa2f991fa2 doc: add note on precedence of options in bitcoin.conf (Torkel Rogstad)
Pull request description:
This was a good addition to the docs, but the PR was closed. So I've cherry-picked the commit and fixed up Russ's comments as well as the linter issue.
ACKs for top commit:
laanwj:
ACK fa2f991fa2
hebasto:
ACK fa2f991fa2, I have reviewed the code and it looks OK, I agree it can be merged.
jamesob:
ACK fa2f991fa2
jonatack:
ACK fa2f991fa2
ryanofsky:
ACK fa2f991fa2. Only suggested changes since previous review.
Tree-SHA512: d8e7bac19e85ad32205652c3c3036766c611cae52e6e3e8af66a2da054659d914dc153d0cf4ace9c0fa7b41f2a8d74d0edd8d83fe7e984b93d70c01a388cf8ec
