The function IsStandardTx() returns rejection reason "bare-multisig" if the
transaction has a bare multisig output and the policy flag fIsBareMultisigStd
is false (set by the boolean command-line argument "-permitbaremultisig" -- for
the unit test, we simply set the global flag variable directly).
5fa28e9903 refactor: Remove unused signal (Hennadii Stepanov)
Pull request description:
`WalletView::showNormalIfMinimized()` signal was introduced in #2872 (dbc0a6aba2).
The only signal emit command was removed in #3144 (2384a2864b)
ACKs for top commit:
promag:
ACK 5fa28e9903.
practicalswift:
ACK 5fa28e9903: nice find
emilengler:
ACK 5fa28e9
jonasschnelli:
utACK 5fa28e9903
Tree-SHA512: 4714acf8c683594d3c00523c7b14bc6b94d469418f0cebe4f4b5266ca0e4c45c80d4caf358739eae9231ee4a69c9c902caeb35f3866b99443cf653f89d6d825b
18b18f8e81 [build] ./configure --enable-werror: add unused-variable (Sjors Provoost)
Pull request description:
The two macOS Travis machines run with `--enable-werror`. This PR adds `-Werror=unused-variable` to the existing `vla`, `switch` and `thread-safety-analysis` checks. This should prevent the need for fixes like b07b07cd87, 26a93bce29, dd777f3e12, 99be644966, fa39f674ae, 16bcc1b823, bb079a0e2c, bdaed47558 and ecf9b25a03 with minimal nuisance.
Thoughts for followups:
* Travis starts these macOS machines fairly late, so we should consider setting `--enable-werror` on earlier machines as well.
* We should encourage the use of `--enable-werror` by developers. Maybe switch it on by default for `--enable-debug`?
* See practicalswift's overview of other checks to consider in #17344
ACKs for top commit:
MarcoFalke:
ACK 18b18f8e81
practicalswift:
ACK 18b18f8e81 -- nice!
Tree-SHA512: 892b471ca5ea547f3c952ac88190cbebf8110cb7aec6f20466aeb312aeb0910bfe990f914e153c40ecb55709c03775ef30770412ad76f9d532ca77055596c582
5e8a56348b test: add unit test for non-standard txs with too large scriptSig (Sebastian Falbesoner)
Pull request description:
Approaches the first missing test of issue #17394: Checks that the function `IsStandardTx()` returns rejection reason `"scriptsig-size"` if any one the inputs' scriptSig is larger than 1650 bytes.
ACKs for top commit:
MarcoFalke:
ACK 5e8a56348b
instagibbs:
ACK 5e8a56348b
Tree-SHA512: 79977b12ddea9438a37cefdbb48cc551e4ad02a8ccfaa2d2837ced9f3a185e2e07cc366c243b9e3c7736245e90e315d7b4110efc6b440c63dbef7ee2c9d78a73
38516f9078 Fix input size assertion in wallet_bumpfee.py (Gregory Sanders)
Pull request description:
I was investigating a curious error for https://github.com/bitcoin/bitcoin/pull/17290 and realized that this check should have caught that error earlier in the test.
The loop is intended to ensure that only a single input exists the entire time until the change output disappears, a single additional bump occurs, then it leaves the loop.
Top commit has no ACKs.
Tree-SHA512: 1d2d6ef535ec2c55f516ee5de11352386ceac6bedaabc6842229a486d9f28d35310ad5f57bfcc1f1e654fc397ecff29ec33256f9b3da897500b7e1635004b63a
edb6b768a4 fix uninitialized variable nMinerConfirmationWindow (NullFunctor)
Pull request description:
It is used for the computation of `BIP9WarningHeight`, and by that time it isn't initialized.
ACKs for top commit:
jnewbery:
utACK edb6b768a
promag:
ACK edb6b768a4, commit description could be cleaned up though.
MarcoFalke:
ACK edb6b768a4, used python3 to do the addition locally 📍
practicalswift:
ACK edb6b768a4, used `clang++ -O2` on the previous version^W^W^W^W^W^W`bc` to verify the addition locally 🏓
Sjors:
Code review ACK edb6b76. Nit: commit description has duplicate text.
Tree-SHA512: 6fa0be0ecfbfd5d537f2c5b4a9333c76530c1f3182f777330cc7939b0496e37b75d8f8810cdaf471a9bd3247b425f2e239578300dfa0d5a87cd14a6ccfafa619
333362991c doc: Explain $LIB in LD_PRELOAD in gitian descriptors (MarcoFalke)
fab9850ef4 scripted-diff: Avoid hardcoded libfaketime dir in gitian (MarcoFalke)
Pull request description:
Without this gitian prints warnings for me:
```
ERROR: ld.so: object '/usr/lib/x86_64-linux-gnu/faketime/libfaketime.so.1' from LD_PRELOAD cannot be preloaded (cannot open shared object file): ignored.
```
```
$ ls /usr/lib/aarch64-linux-gnu/faketime/libfaketime.so.1
/usr/lib/aarch64-linux-gnu/faketime/libfaketime.so.1
```
ACKs for top commit:
laanwj:
ACK 333362991c
Tree-SHA512: 3e7c4471b69c2ae38c29d0cc0db8b9eae0912085299d7f5ac67eeb4b6a2fdc7eb23d806eeeae0b0c2da22d6d1ba82513cab23652876b97aada9928b2c7d38e7e
49997813a4 test: check custom ancestor limit in mempool_packages.py (Sebastian Falbesoner)
Pull request description:
The functional test `mempool_packages.py` starts one node with default ancestor/descendant limit settings and one with a custom, reduced ancestor limit (currently `-limitancestorcount=5`). The effect of the latter had not been tested yet though. This is approached in this PR by checking on the expected mempool contents of node1 after the node0 ancestor tests are done, via the following three conditions:
- the # of txs in the node1 mempool is equal to the the limit
- all txs in node1 mempool are a subset of txs in node0 mempool
- the node1 mempool txs match the start of the constructed tx-chain
Note that this still doesn't *fully* check the expected mempool of node1 (e.g. that it isn't influenced by `prioritisetransaction` RPC on node0), hence I add another TODO. In the future it would make sense to also set a custom descendant limit when the second TODO about checking node1's mempool is approached: 89e93135ae/test/functional/mempool_packages.py (L228)
ACKs for top commit:
MarcoFalke:
ACK 49997813a4👲
Tree-SHA512: d3a1d19fb49731238ad08ee7c02e2fa81a227e3b4ef3340d68598de42ddb62be9161134f6b8e08fa76b8c9faa02fecfa01111159642e20e9f358292a757b7608
To test the custom ancestor limit on node1 (passed by the argument
-limitancestorcount), we check for three conditions:
-> the # of txs in the node1 mempool is equal to the the limit
-> all txs in node1 mempool are a subset of txs in node0 mempool
-> the node1 mempool txs match the start of the constructed tx-chain
eb880f092b fix Typo: "merkelRoot" -> "merkleRoot" (ianliu)
Pull request description:
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improve coverage are always welcome.
* All other changes should have accompanying unit tests (see `src/test/`) or
functional tests (see `test/`). Contributors should note which tests cover
modified code. If no tests exist for a region of modified code, new tests
should accompany the change.
* Bug fixes are most welcome when they come with steps to reproduce or an
explanation of the potential issue as well as reasoning for the way the bug
was fixed.
* Features are welcome, but might be rejected due to design or scope issues.
If a feature is based on a lot of dependencies, contributors should first
consider building the system outside of Bitcoin Core, if possible.
* Refactoring changes are only accepted if they are required for a feature or
bug fix or otherwise improve developer experience significantly. For example,
most "code style" refactoring changes require a thorough explanation why they
are useful, what downsides they have and why they *significantly* improve
developer experience or avoid serious programming bugs. Note that code style
is often a subjective matter. Unless they are explicitly mentioned to be
preferred in the [developer notes](/doc/developer-notes.md), stylistic code
changes are usually rejected.
-->
<!--
Bitcoin Core has a thorough review process and even the most trivial change
needs to pass a lot of eyes and requires non-zero or even substantial time
effort to review. There is a huge lack of active reviewers on the project, so
patches often sit for a long time.
-->
ACKs for top commit:
practicalswift:
ACK eb880f092b but please change from `merkleRootofHashes` to `merkleRootOfHashes`
Tree-SHA512: ada9edceee19da5678bf35e1258163e7102fe176dc5cf40acaa1468fa8b2801494f8bf65d5359dcd0054fbc22f07fdc98d6208cfdb54dd9171fd45c89d71e098
29eb039252 Moves vcpkg list to a text file and updates the appveyor job and readme to use it. (Aaron Clauson)
Pull request description:
#17364 attempted to save a couple of minutes by skipping the `vcpkg` steps if the vcpkg install directory was already cached.
The discussion in #15382 highlights the approach used in #17364 does not accommodate adding a new package.
~~This PR improves the approach to individually check whether each vcpg package is installed rather than checking for the existence of the vcpkg install directory.~~
This PR moves the list of required vcpkg packages into a separate file and uses changes to that file to invalidate the appveyor cache. Whenever the cache is invalidated the vcpkg sources will be updated, the vcpkg binary built and the required packages installed from the latest port files.
ACKs for top commit:
MarcoFalke:
ACK 29eb039252
Tree-SHA512: 0c2a170f4e4b47ca0f9cef14f1e3892001b441a6d84f50bf5fd8a26bc4cdbd9358dfce7ef180d37150262e849650e9857d6b2bcd686964b963c3de6cd708a2f3
e2c03c1156 doc: Add relase note for db→walletdb rename (Wladimir J. van der Laan)
4c1d263d93 scripted-diff: Change `BCLog::DB` to `BCLog::WALLETDB` (Wladimir J. van der Laan)
6b42b3ba90 Rename `db` log category to `walletdb` (like `coindb`) (Wladimir J. van der Laan)
Pull request description:
Rename the `db` log category to `walletdb` (in the style of, and to distinguish from `coindb`). Deprecate (but still accept) '-debug=db'.
Second commit is a scripted commit that changes the enum item name.
ACKs for top commit:
hebasto:
ACK e2c03c1156, tested on Linux Mint 19.2:
Tree-SHA512: a044de6f9a70e735cbb1caa4ed6bf75bc2269b2d5bc3241a25b6a6d69c1fc1d83456e252b431388ae61f4821e4fc06ecc1b634816ceadbe9a3c0e494bee6c11e
083c954b02 Add settings_tests (Russell Yanofsky)
7f40528cd5 Deduplicate settings merge code (Russell Yanofsky)
9dcb952fe5 Add util::Settings struct and helper functions. (Russell Yanofsky)
e2e37cfe8a Remove includeconf nested scope (Russell Yanofsky)
5a84aa880f Rename includeconf variables for clarity (Russell Yanofsky)
dc8e1e7548 Clarify emptyIncludeConf logic (Russell Yanofsky)
Pull request description:
This is a refactoring-only change that makes it easier to add a new settings source.
This PR doesn't change behavior. The [`util_ArgsMerge`](deb2327b43/src/test/util_tests.cpp (L626-L822)) and [`util_ChainMerge`](deb2327b43/src/test/util_tests.cpp (L843-L924)) tests added in #15869 and #15988 were written specifically to confirm that ArgsManager settings are parsed, merged, and returned the same way before and after this change.
This change:
- Makes it easier to add new settings sources that can get merged with existing sources (see 70675c3e4975203ad6222ba2b00c83b4e4213793 from #15935).
- Separates parsing of settings from merging of settings, and deduplicates merging code so it doesn't happen five different places ([GetArg](c459c5f701/src/util/system.cpp (L221-L244)), [GetNetBoolArg](c459c5f701/src/util/system.cpp (L255-L261)), [GetArgs](c459c5f701/src/util/system.cpp (L460-L467)), [IsArgNegated](c459c5f701/src/util/system.cpp (L482-L491)), [GetUnsuitableSectionOnlyArgs](c459c5f701/src/util/system.cpp (L343-L352))) in inconsistent ways.
- Documents and tests current strange merging behaviors, so they be cleaned up in the future if resulting code simplifications and UX improvements warrant loss of backwards compatibility. The newly documented behaviors are: command line [ignored arguments](69d44f3cc7/src/util/system.cpp (L323-L326)) and [more ignored arguments](69d44f3cc7/src/util/settings.cpp (L67-L72)), and config file [reverse precedence](69d44f3cc7/src/util/settings.cpp (L61-L65)), [inconsistently applied top-level settings](69d44f3cc7/src/util/settings.cpp (L55-L59)), and [zombie values](69d44f3cc7/src/util/settings.cpp (L101-L108)).
The original motivation for this change was to make it easy to add a new persistent setting source without introducing more bugs and inconsistencies. Two commits building on top of this to add a persistent `-wallet` setting are pretty straightforward and show how the new code can be extended:
* 70675c3e4975203ad6222ba2b00c83b4e4213793 from #15935 – _Add \<datadir>/settings.json persistent settings storage_
* 04c80c40df9fc6f4734ba238ea7f65607cf88089 from #15937 – _Add loadwallet and createwallet RPC load_on_startup options_
ACKs for top commit:
ariard:
ACK 083c954
jnewbery:
ACK 083c954b02
jamesob:
ACK 083c954b02
Tree-SHA512: 5d106746a44d64d3963c4ef3f4a2fa668a4bedcc9018d3ea12c86beae2fda48a0b036241665837f68685712366f70f2e1faba84d193fa1f456013503097b7659
faf757a125 ci: Guess the native host when not cross compiling (MarcoFalke)
fa8a60bce9 ci: Run non-cross-compile builds natively (MarcoFalke)
fa56bcbb01 ci: Run CI_WAIT only on travis (MarcoFalke)
Pull request description:
non-cross-compile ci builds should not hardcode an architecture, so they can be run on any ci system
ACKs for top commit:
laanwj:
re-ACK faf757a125
Tree-SHA512: 97f86ad411e98c6317a62f829bee26c16dbe3fa54d8ac013018f7669b653d7d6d750740b2ecfb7175195d5fffc701ce503b0d11802b97af30904b51bb23f2073
fad1de66a2 wallet: Remove unused boost::this_thread::interruption_point (MarcoFalke)
Pull request description:
`BerkeleyEnvironment::Open` is only called from the main thread (init) or an http rpc thread, neither of which can be interrupted, so remove the useless interruption point.
`BerkeleyEnvironment{}` is only used in tests, which run in a single process/thread, so remove the useless interruption point.
ACKs for top commit:
laanwj:
ACK fad1de66a2
fanquake:
ACK fad1de66a2
Tree-SHA512: dacd8398e966e4a6ce5cf7d3ed821c9c267eff40b14c0635085441647cdb72d1642807f89355419f1710f814c7963e35a10d102d0b985c7198261dfc736256f8
0b75a7f068 wallet: Reuse existing batch in CWallet::SetUsedDestinationState (João Barbosa)
01f45dd00e wallet: Avoid recursive lock in CWallet::SetUsedDestinationState (João Barbosa)
Pull request description:
This PR makes 2 distinct changes around `CWallet::SetUsedDestinationState`:
- 1st the recursive lock is removed and now it requires the lock to be held;
- 2nd change is to support, in the best case, just a wallet database flush when transaction is added to the wallet.
ACKs for top commit:
achow101:
ACK 0b75a7f068
MarcoFalke:
ACK 0b75a7f068
ryanofsky:
Code review ACK 0b75a7f068. Code changes looks fine but PR description should be updated to say what benefits of the change are. I might have missed something, but I didn't see a place where multiple batches were used previously and a single batch was used now. So the main benefit of this change appears to be removing a recursive lock? And maybe moving toward a consistent convention for passing batch instances?
Tree-SHA512: abcf23a5850d29990668db20d6f624cca3e89629cc9ed003e0d05cde1b58ab2ff365034f156684ad13e55764b54c6c0c2bc7d5f96b8af7dc5e45a3be955d6b15
36b68de5b2 Remove getBlockDepth method from Chain::interface (Antoine Riard)
b66c429c56 Remove locked_chain from GetDepthInMainChain and its callers (Antoine Riard)
0ff03871ad Use CWallet::m_last_block_processed_height in GetDepthInMainChain (Antoine Riard)
f77b1de16f Only return early from BlockUntilSyncedToCurrentChain if current tip is exact match (Antoine Riard)
769ff05e48 Refactor some importprunedfunds checks with guard clause (Antoine Riard)
5971d3848e Add block_height field in struct Confirmation (Antoine Riard)
9700fcb47f Replace CWalletTx::SetConf by Confirmation initialization list (Antoine Riard)
5aacc3eff1 Add m_last_block_processed_height field in CWallet (Antoine Riard)
10b4729e33 Pass block height in Chain::BlockConnected/Chain::BlockDisconnected (Antoine Riard)
Pull request description:
Work starter to remove Chain::Lock interface by adding m_last_block_processed_height in CWallet and m_block_height in CMerkleTx to avoid GetDepthInMainChain having to keep a lock . Once this one done, it should ease work to wipe out more cs_main locks from wallet code.
I think it's ready for a first round of review before to get further.
- `BlockUntilSyncedToCurrent` : restrain isPotentialTip to isTip because we want to be sure that wallet see BlockDisconnected callbacks if its height differs from the Chain one. It means during a reorg, an RPC could return before the BlockDisconnected callback had been triggered. This could cause a tx that had been included in the disconnected block to be displayed as confirmed, for example.
~~- `AbandonTransaction` : in case of conflicted tx (nIndex = -1), we set its m_block_height to the one of conflicting blocks, but if this height is superior to CWallet::m_last_block_processed_height, that means tx isn't conflicted anymore so we return 0 as tx is again unconfirmed~~ After #16624, we instead rely on Confirmation.
~~- `AddToWalletIfInvolvingMe`: in case of block disconnected, transactions are added to mempool again, so we need to replace old txn in `mapWallet` with a height set to zero so we remove check on block_hash.IsNull~~ Already done in #16624
ACKs for top commit:
jnewbery:
@jkczyz you've ACKed an intermediate commit (github annoyingly orders commits in date order, not commit order). Did you mean to ACK the final commit in this branch (36b68de5b2).
jkczyz:
> @jkczyz you've ACKed an intermediate commit (github annoyingly orders commits in date order, not commit order). Did you mean to ACK the final commit in this branch ([36b68de](36b68de5b2)).
meshcollider:
utACK 36b68de5b2
ryanofsky:
Code review ACK 36b68de5b2. Changes since last review: new jkczyz refactor importprunedfunds commit, changed BlockUntilSyncedToCurrentChainChanges commit title and description, changed Confirmation struct field order and line-wrapped comment
jnewbery:
utACK 36b68de5b2
promag:
Code review ACK 36b68de5b2.
Tree-SHA512: 08b89a0bcc39f67c82a6cb6aee195e6a11697770c788ba737b90986b4893f44e90d1ab9ef87239ea3766508b7e24ea882b7199df41173ab27a3d000328c14644
Get rid of settings merging code in util/system.cpp repeated 5 places,
inconsistently:
- ArgsManagerHelper::GetArg
- ArgsManagerHelper::GetNetBoolArg
- ArgsManager::GetArgs
- ArgsManager::IsArgNegated
- ArgsManager::GetUnsuitableSectionOnlyArgs
Having settings merging code separated from parsing simplifies parsing somewhat
(for example negated values can simply be represented as false values instead
of partially cleared or emply placeholder lists).
Having settings merge happen one place instead of 5 makes it easier to add new
settings sources and harder to introduce new inconsistencies in the way
settings are merged.
This commit does not change behavior in any way.
Implement merging of settings from different sources (command line and config
file) separately from parsing code in system.cpp, so it is easier to add new
sources.
Document current inconsistent merging behavior without changing it.
This commit only adds new settings code without using it. The next commit calls
the new code to replace existing code in system.cpp.
Co-authored-by: John Newbery <john@johnnewbery.com>
