This speeds up the test significantly, which helps when
running it repeatedly.
Suggest reviewing the diff with:
colorMoved = dimmed-zebra
colorMovedWs = allow-indentation-change
At verification time, the to be precomputed data can be inferred from
the transaction itself. For signing, the necessary witnesses don't
exist yet, so just permit precomputing everything in that case.
This provides a means to pass in a PrecomputedTransactionData object to
the MutableTransactionSignatureCreator, allowing the prevout data to be
passed into the signature hashers. It is also more efficient.
This data structures stores all information necessary for spending a taproot
output (the internal key, the Merkle root, and the control blocks for every
script leaf).
It is added to signing providers, and populated by the tr() descriptor.
fbf485c9b2 Allow tr() import only when Taproot is active (Andrew Chow)
Pull request description:
To avoid issues around fund loss, only allow descriptor wallets to import `tr()` descriptors after taproot has activated.
ACKs for top commit:
sipa:
utACK fbf485c9b2
fjahr:
Code review ACK fbf485c9b2
laanwj:
Code review ACK fbf485c9b2
prayank23:
utACK fbf485c9b2
Tree-SHA512: 83c43376515eea523dbc89bc5a0fde53e54aec492e49a40c2a33d80fc94aac459e232ae07b024b4bd75b58078c8d090bc7a2d69541c5d3d4834d2f4cfc9c8208
f507681baa qt: Connect WalletView signal to BitcoinGUI slot directly (Hennadii Stepanov)
bd50ff9290 qt: Drop redundant OverviewPage::handleOutOfSyncWarningClicks slot (Hennadii Stepanov)
793f19599b qt: Drop redundant WalletView::requestedSyncWarningInfo slot (Hennadii Stepanov)
Pull request description:
This PR:
- removes slots whose only job is to emit a signal, since we can use the signal as a slot
- connects the`WalletView::outOfSyncWarningClicked` signal to the `BitcoinGUI::showModalOverlay` slot directly, and removes intermediate `WalletFrame` slot and signal
- split from #29
This PR does not change behavior.
ACKs for top commit:
Talkless:
tACK f507681baa, tested on Debian Sid with Qt 5.15.2, no any behavioral changes noticed.
promag:
Code review ACK f507681baa.
Tree-SHA512: cd636a7e61881b2cbee84d5425d2107a8e39683b8eb32d79dc9ea942db55d5c1979be2f70da1660eaee5de622d10ed5a92f11fc2351de21b84324b10b23d0c96
fa334b4054 refactor: Pass block reference instead of pointer to PeerManagerImpl::BlockRequested (MarcoFalke)
Pull request description:
This allows to remove an assert and at the same time make it more obvious that the block is never nullptr.
Also, add missing `{}` while touching the function.
ACKs for top commit:
jnewbery:
Code review ACK fa334b4054
mjdietzx:
crACK fa334b4054
theStack:
Code review ACK fa334b4054
Tree-SHA512: 9733d3e20e048fcb2ac7510eae3539ce8aaa7397bd944a265123f1ffd90e15637cdaad19dba16f76d83f3f0d1888f1b7014c191bb430e410a106c49ca61a725c
a92485b2c2 addrman: use unordered_map instead of map (Vasil Dimov)
Pull request description:
`CAddrMan` uses `std::map` internally even though it does not require
that the map's elements are sorted. `std::map`'s access time is
`O(log(map size))`. `std::unordered_map` is more suitable as it has a
`O(1)` access time.
This patch lowers the execution times of `CAddrMan`'s methods as follows
(as per `src/bench/addrman.cpp`):
```
AddrMan::Add(): -3.5%
AddrMan::GetAddr(): -76%
AddrMan::Good(): -0.38%
AddrMan::Select(): -45%
```
ACKs for top commit:
jonatack:
ACK a92485b2c2
achow101:
ACK a92485b2c2
hebasto:
re-ACK a92485b2c2, only suggested changes and rebased since my [previous](https://github.com/bitcoin/bitcoin/pull/18722#pullrequestreview-666663681) review.
Tree-SHA512: d82959a00e6bd68a6c4c5a265dd08849e6602ac3231293b7a3a3b7bf82ab1d3ba77f8ca682919c15c5d601b13e468b8836fcf19595248116635f7a50d02ed603
f47e802839 Rearrange fillPSBT arguments (Russell Yanofsky)
Pull request description:
Move fillPSBT inout argument before output-only arguments. This is a nice thing to do to keep the interface style [consistent](https://google.github.io/styleguide/cppguide.html#Inputs_and_Outputs). But motivation is to work around a current limitation of the libmultiprocess code generator (which figures out order of inout parameters by looking at input list, but more ideally would use the output list).
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10). The commit was first part of larger PR #10102.
ACKs for top commit:
achow101:
ACK f47e802839
theStack:
Code-review ACK f47e802839
Tree-SHA512: 1787af3031ff7ed6b519f3b93054d8b257af96a3380a476a6dab0f759329039ecc5d624b785c5c2d14d594fc852dd81c626880c775c691ec9c79b7b3dbcfb257
6f994882de validation: Farewell, global Chainstate! (Carl Dong)
972c5166ee qt/test: Reset chainman in ~ChainstateManager instead (Carl Dong)
6c3b5dc0c1 scripted-diff: tree-wide: Remove all review-only assertions (Carl Dong)
3e82abb8dd tree-wide: Remove stray review-only assertion (Carl Dong)
f323248aba qt/test: Use existing chainman in ::TestGUI (can be scripted-diff) (Carl Dong)
6c15de129c scripted-diff: wallet/test: Use existing chainman (Carl Dong)
ee0ab1e959 fuzz: Initialize a TestingSetup for test_one_input (Carl Dong)
0d61634c06 scripted-diff: test: Use existing chainman in unit tests (Carl Dong)
e197076219 test: Pass in CoinsTip to ValidateCheckInputsForAllFlags (Carl Dong)
4d99b61014 test/miner_tests: Pass in chain tip to CreateBlockIndex (Carl Dong)
f0dd5e6bb4 test/util: Use existing chainman in ::PrepareBlock (Carl Dong)
464c313e30 init: Use existing chainman (Carl Dong)
Pull request description:
Based on: #21767
à la Mr. Sandman
```
Mr. Chainman, bring me a tip (bung, bung, bung, bung)
Make it the most work that I've ever seen (bung, bung, bung, bung)
Rewind old tip till we're at the fork point (bung, bung, bung, bung)
Then tell it that it's time to call Con-nectTip
Chainman, I'm so alone (bung, bung, bung, bung)
No local objects to call my own (bung, bung, bung, bung)
Please make sure I have a ref
Mr. Chainman, bring me a tip!
```
This is the last bundle in the #20158 series. Thanks everyone for their diligent review.
I would like to call attention to https://github.com/bitcoin/bitcoin/issues/21766, where a few leftover improvements were collated.
- Remove globals:
- `ChainstateManager g_chainman`
- `CChainState& ChainstateActive()`
- `CChain& ChainActive()`
- Remove all review-only assertions.
ACKs for top commit:
jamesob:
reACK 6f994882de based on the contents of
ariard:
Code Review ACK 6f99488.
jnewbery:
utACK 6f994882de
achow101:
Code Review ACK 6f994882de
ryanofsky:
Code review ACK 6f994882de.
Tree-SHA512: 4052ea79360cf0efd81ad0ee3f982e1d93aab1837dcec75f875a56ceda085de078bb3099a2137935d7cc2222004ad88da94b605ef5efef35cb6bc733725debe6
as EraseLastKElements() called in the next line performs the same operation.
Thanks to Martin Zumsande (lightlike) for seeing this while reviewing.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
493fb47c57 Make SetupServerArgs callable without NodeContext (Russell Yanofsky)
Pull request description:
`bitcoin-gui` code needs to call `SetupServerArgs` but will not have a `NodeContext` object if it is communicating with an external `bitcoin-node` process, so this just passes `ArgsManager` directly.
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10). The commit was first part of larger PR #10102.
ACKs for top commit:
MarcoFalke:
review ACK 493fb47c57
Tree-SHA512: 94cda4350113237976e32f1935e3602d1e6ea90c29c4434db2094be70dddf4b63702c3094385258bdf1c3e5b52c7d23bbc1f0282bdd4965557eedd5aef9a0fd4
fa72fce7c9 test: Use ConnmanTestMsg from test lib in denialofservice_tests (MarcoFalke)
Pull request description:
This allows to remove code.
Also, required for https://github.com/bitcoin/bitcoin/pull/18470
ACKs for top commit:
mjdietzx:
crACK fa72fce7c9👍👍
fanquake:
ACK fa72fce7c9
Tree-SHA512: 12aa68cde697c0f7c25d60bb0c02783e5462eb3ba39947b0d94a7798bc278e7d5f092f3ab2a3d0547947c3502cde7c4a599419055a57f78ef1f70f9f637e14c7
There are some mutable, global state variables that are currently reset
by UnloadBlockIndex such as pindexBestHeader which should be cleaned up
whenever the ChainstateManager is unloaded/reset/destructed/etc.
Not cleaning them up leads to bugs like a use-after-free that happens
like so:
1. At the end of a test, ChainstateManager is destructed, which also
destructs BlockManager, which calls BlockManager::Unload to free all
CBlockIndexes in its BlockMap
2. Since pindexBestHeader is not cleaned up, it now points to an invalid
location
3. Another test starts to init, and calls LoadGenesisBlock, which calls
AddToBlockIndex, which compares the genesis block with an invalid
location
4. Cute puppies perish by the hundreds
Previously, for normal codepaths (e.g. bitcoind), we relied on the fact
that our program will be unloaded by the operating system which
effectively resets these variables. The one exception is in QT tests,
where these variables had to be manually reset.
Since now ChainstateManager is no longer a global, we can just put this
logic in its destructor to make sure that callers are always correct.
Over time, we should probably move these mutable global state variables
into ChainstateManager or CChainState so it's easier to reason about
their lifecycles.
Move fillPSBT input-output argument before output-only arguments. This is a
temporary workaround which can go away with improvements to libmultiprocess
code generator. Currently code generator figures out order of input-output
parameters by looking at input list, but it would make more sense for it to
take order from output list, so input-only parameters still have to be first
but there is more flexibility for the other parameters.
2f4ad6b7ef scripted-diff: rename MarkBlockAs functions (John Newbery)
2c45f832e8 [net processing] Tidy up MarkBlockAsReceived() (John Newbery)
6299350733 [net processing] Add IsBlockRequested() function (John Newbery)
4e90d2dd0e [net processing] Remove QueuedBlock.hash (John Newbery)
156a19ee6a scripted-diff: rename nPeersWithValidatedDownloads (John Newbery)
b03de9c753 [net processing] Remove CNodeState.nBlocksInFlightValidHeaders (John Newbery)
b4e29f2436 [net processing] Remove QueuedBlock.fValidatedHeaders (John Newbery)
85e058b191 [net processing] Remove unnecessary hash arg from MarkBlockAsInFlight() (John Newbery)
Pull request description:
The QueuedBlock struct contains a `fValidatedHeaders` field that indicates whether we have already validated a header for the requested block. Since headers-first syncing, we only request blocks where the header is already validated, so `fValidatedHeaders` is always true. Remove it and clean up the logic that uses that field.
Likewise, QueuedBlock contains a `hash` field that is set to the block hash. Since headers-first syncing, we always have a CBlockIndex, which contains the block hash, so remove the redundant `hash` field.
Tidy up the logic and rename functions to better indicate what they're doing.
ACKs for top commit:
mjdietzx:
crACK 2f4ad6b7ef
sipa:
utACK 2f4ad6b7ef
MarcoFalke:
review ACK 2f4ad6b7ef📊
Tree-SHA512: 3d31d2bcb4d35d0fdb7c1da624c2878203218026445e8f76c4a2df68cc7183ce0e7d0c47c7c0a3242e55efaca7c9f5532b683cf6ec7c03d23fa83764fdb82fd2
3f05a9e681 zmq: use msg: prefix over errno= in zmqError (fanquake)
9a7cb57bbc zmq: use std::string in zmqError() (fanquake)
Pull request description:
This is two minor changes. The first is to change `zmqError` to take a `const std::string&` instead of a `const char*`. The second is to change the second portion of `zmqError` to print `msg: message` rather than `errno=message`, given that `zmq_strerror` returns a message. To me, this seems more readable / useful than output like: `Error: Unable to initialize context errno=No such file or directory`.
ACKs for top commit:
practicalswift:
cr ACK 3f05a9e681
instagibbs:
utACK 3f05a9e681
theStack:
Code-Review ACK 3f05a9e681
Tree-SHA512: 197cf381e8b3ced271d0e575e0c6d8e5e9ed93c4b284338b17873c5232eaabe64d6c4b66e1aeb5e76befc89e316abae2b28b7fd760f178481d7b9f4e3f85da67
e.g.:
In file included from /usr/local/include/boost/test/test_tools.hpp:46:
/usr/local/include/boost/test/tools/old/impl.hpp:107:17: error: comparison of integers of different signs: 'const unsigned int' and 'const int' [-Werror,-Wsign-compare]
return left == right;
~~~~ ^ ~~~~~
/usr/local/include/boost/test/tools/old/impl.hpp:130:16: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl<unsigned int, int>' requested here
return equal_impl( left, right );
^
/usr/local/include/boost/test/tools/old/impl.hpp:145:16: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl_frwd::call_impl<unsigned int, int>' requested here
return call_impl( left, right, left_is_array() );
^
/usr/local/include/boost/test/tools/old/impl.hpp:92:50: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl_frwd::operator()<unsigned int, int>' requested here
BOOST_PP_REPEAT( BOOST_TEST_MAX_PREDICATE_ARITY, IMPL_FRWD, _ )
^
/usr/local/include/boost/preprocessor/repetition/repeat.hpp:30:26: note: expanded from macro 'BOOST_PP_REPEAT'
^
/usr/local/include/boost/preprocessor/cat.hpp:22:32: note: expanded from macro 'BOOST_PP_CAT'
^
/usr/local/include/boost/preprocessor/cat.hpp:29:34: note: expanded from macro 'BOOST_PP_CAT_I'
^
<scratch space>:153:1: note: expanded from here
BOOST_PP_REPEAT_1
^
test/streams_tests.cpp:122:5: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::check_frwd<boost::test_tools::tt_detail::equal_impl_frwd, unsigned int, int>' requested here
BOOST_CHECK_EQUAL(varint, 54321);
^
/usr/local/include/boost/test/tools/old/impl.hpp:107:17: error: comparison of integers of different signs: 'const unsigned long long' and 'const long' [-Werror,-Wsign-compare]
return left == right;
~~~~ ^ ~~~~~
/usr/local/include/boost/test/tools/old/impl.hpp:130:16: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl<unsigned long long, long>' requested here
return equal_impl( left, right );
^
/usr/local/include/boost/test/tools/old/impl.hpp:145:16: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl_frwd::call_impl<unsigned long long, long>' requested here
return call_impl( left, right, left_is_array() );
^
/usr/local/include/boost/test/tools/old/impl.hpp:92:50: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl_frwd::operator()<unsigned long long, long>' requested here
BOOST_PP_REPEAT( BOOST_TEST_MAX_PREDICATE_ARITY, IMPL_FRWD, _ )
^
/usr/local/include/boost/preprocessor/repetition/repeat.hpp:30:26: note: expanded from macro 'BOOST_PP_REPEAT'
^
/usr/local/include/boost/preprocessor/cat.hpp:22:32: note: expanded from macro 'BOOST_PP_CAT'
^
/usr/local/include/boost/preprocessor/cat.hpp:29:34: note: expanded from macro 'BOOST_PP_CAT_I'
^
<scratch space>:161:1: note: expanded from here
BOOST_PP_REPEAT_1
^
test/serfloat_tests.cpp:41:5: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::check_frwd<boost::test_tools::tt_detail::equal_impl_frwd, unsigned long long, long>' requested here
BOOST_CHECK_EQUAL(TestDouble(std::numeric_limits<double>::infinity()), 0x7ff0000000000000);
^
zmq_strerror() converts the passed errno into a description, meaning
currently you have output like: "errno=No such file or directory".
Using msg: would seem to make more sense here.
e60cd26ad4 Do not load external signers wallets when unsupported (Andrew Chow)
Pull request description:
When external signer support is not compiled, do not load external signer wallets.
Alternative to #22168.
ACKs for top commit:
promag:
Tested ACK e60cd26ad4.
meshcollider:
Code review ACK e60cd26ad4
Tree-SHA512: aed2d0038f448c2f89c6b48f412b106e63c9ed20e748e69aae21fb58c33fc7e4fa73375a52372c73788669eb2b968a8da6b022c65658fa4484f5bbcf205b1b15
d44a261acf Fix issues when `walletdir` is root directory (unknown)
Pull request description:
+ Remove one character less from wallet path
+ After testing lot of random strings with special chars in `wallet_name`, I found that the issue was not related to special characters in the name. Reviewing PR https://github.com/bitcoin/bitcoin/pull/21907 helped me resolve the issue.
**Real issue**: If the path mentioned in `walletdir` is a root directory, first character of the wallet name or path is removed
**Solution**: `if` statement to check `walletdir` is a root directory
Fixes: https://github.com/bitcoin/bitcoin/issues/21510https://github.com/bitcoin/bitcoin/issues/21501
Related PR: https://github.com/bitcoin/bitcoin/pull/20080
Consider the wallet directories `w1` and `w2` saved in `D:\`. Run `bitcoind.exe -walletdir=D:\`, Results for `bitcoin-cli.exe listwalletdir`:
Before this PR:
```
{
"wallets": [
{
"name": "1"
},
{
"name": "2"
}
]
}
```
After this PR:
```
"wallets": [
{
"name": "w1"
},
{
"name": "w2"
}
]
}
```
ACKs for top commit:
ryanofsky:
Code review ACK d44a261acf
meshcollider:
utACK d44a261acf
Tree-SHA512: b09b00f727407e3771c8694861dae1bfd29d97a0d51ddcb5d9c0111dc618b3fff2f75829cbb4361c54457ee564e94fcefd9e2928262a1c918a2b6bbad724eb55
96c2c9520e scripted-diff: Rename SelectCoinsMinConf to AttemptSelection (Andrew Chow)
b583f73354 Move vin filling to before final fee setting (Andrew Chow)
d39cac0547 Set m_subtract_fee_outputs during recipients vector loop (Andrew Chow)
364e0698a5 Move variable initializations to where they are used (Andrew Chow)
32ab430651 Move recipients vector checks to beginning of CreateTransaction (Andrew Chow)
cd1d6d3324 Rename nSubtractFeeFromAmount in CreateTransaction (Andrew Chow)
dac21c793f Rename nValue and nValueToSelect (Andrew Chow)
d2aee3bbc7 Remove extraneous scope in CreateTransactionInternal (Andrew Chow)
b2995963b5 Move cs_wallet lock in CreateTransactionInternal to top of function (Andrew Chow)
Pull request description:
#17331 did some refactors and cleanup of `CreateTransactionInternal` to make it easier to understand, however it is still a bit convoluted even though it doesn't have to be. This PR does additional cleanup and refactoring to `CreateTransactionInternal` so that it is easier to understand. Some unnecessary code was removed, some variables moved around to where they matter, and several indents removed.
ACKs for top commit:
glozow:
reACK 96c2c95
ryanofsky:
Code review ACK 96c2c9520e also acked previously (was reverted).
meshcollider:
re-utACK 96c2c9520e
Tree-SHA512: 3dba67ed436968a07bfd82d435d566ad74e116c6e50ac9baed7144a46ad5c0f630b1ba59d91e8e8972ac2af559d7c0576f0560f09684d2ab20fad6689902866f
1c4b456e1a gui: send using external signer (Sjors Provoost)
24815c6309 gui: wallet creation detects external signer (Sjors Provoost)
3f845ea299 node: add externalSigners to interface (Sjors Provoost)
62ac119f91 gui: display address on external signer (Sjors Provoost)
450cb40a34 wallet: add displayAddress to interface (Sjors Provoost)
eef8d64529 gui: create wallet with external signer (Sjors Provoost)
6cdbc83e93 gui: add external signer path to options dialog (Sjors Provoost)
Pull request description:
Big picture overview in [this gist](https://gist.github.com/Sjors/29d06728c685e6182828c1ce9b74483d).
This PR adds GUI support for external signers, based on the since merged bitcoin/bitcoin#16546 (RPC).
The UX isn't amazing - especially the blocking calls - but it works.
First we adds a GUI setting for the signer script (e.g. path to HWI):
<img width="625" alt="Schermafbeelding 2019-08-05 om 19 32 59" src="https://user-images.githubusercontent.com/10217/62483415-e1ff1680-b7b7-11e9-97ca-8d2ce54ca1cb.png">
Then we add an external signer checkbox to the wallet creation dialog:
<img width="374" alt="Schermafbeelding 2019-11-07 om 19 17 23" src="https://user-images.githubusercontent.com/10217/68416387-b57ee000-0194-11ea-9730-127d60273008.png">
It's checked by default if HWI detects a device. It also grabs the name. It then creates a fresh wallet and imports the keys.
You can verify an address on the device (blocking...):
<img width="673" alt="Schermafbeelding 2019-08-05 om 19 29 22" src="https://user-images.githubusercontent.com/10217/62483560-43bf8080-b7b8-11e9-9902-8a036116dc4b.png">
Sending, including coin selection, Just Works(tm) as long the device is present.
~External signer support is enabled by default when the GUI is configured and Boost::Process is present.~
External signer support remains disabled by default, see https://github.com/bitcoin/bitcoin/pull/21935.
ACKs for top commit:
achow101:
Code Review ACK 1c4b456e1a
hebasto:
ACK 1c4b456e1a, tested on Linux Mint 20.1 (Qt 5.12.8) with HWW `2.0.2-rc.1`.
promag:
Tested ACK 1c4b456e1a but rebased with e033ca1379, with HWI 2.0.2, with Nano S and Nano X.
meshcollider:
re-code-review ACK 1c4b456e1a
Tree-SHA512: 3503113c5c69d40adb6ce364d8e7cae23ce82d032a00474ba9aeb6202eb70f496ef4a6bf2e623e5171e524ad31ade7941a4e0e89539c64518aaec74f4562d86b
2eb0eeda39 validation: document lack of inherited signaling in RBF policy (Antoine Riard)
906b6d9da6 test: Extend feature_rbf.py with no inherited signaling (Antoine Riard)
Pull request description:
Contrary to BIP125 or other full-node implementation (e.g btcd), Bitcoin Core's mempool policy doesn't implement inherited signaling.
This PR documents our mempool behavior on this and add a test demonstrating the case.
ACKs for top commit:
jonatack:
ACK 2eb0eeda39
benthecarman:
ACK 2eb0eeda39
Tree-SHA512: d41453d3b49bae3c1eb532a968f43bc047084913bd285929d4d9cba142777ff2be38163d912e28dfc635f4ecf446de68effad799c6e71be52f81e83410c712fb
fa13f34bf3 fuzz: Increase branch coverage of the float fuzz target (MarcoFalke)
fad0c58c3e fuzz: Remove confusing return keyword from CallOneOf (MarcoFalke)
Pull request description:
Currently the branch coverage for the float fuzz target is only 50% : https://marcofalke.github.io/btc_cov/fuzz.coverage/src/test/fuzz/float.cpp.gcov.html
This is caused by the Fuzzed Data Provider only picking "nice" floats.
ACKs for top commit:
practicalswift:
cr ACK fa13f34bf3: patch looks correct
Tree-SHA512: 326822515e9a1c77647d41eab9a96185a3b320914d9264730fa72ffb76c2bf3dc5bf72cf6cd9beef14f4f032358d76a976860bf3e2418ae61943cf926c0ea086
5c7ee1b2da libsecp256k1 no longer has --with-bignum= configure option (Pieter Wuille)
bdca9bcb6c Squashed 'src/secp256k1/' changes from 3967d96bf1..efad3506a8 (Pieter Wuille)
cabb566123 Disable certain false positive warnings for libsecp256k1 msvc build (Pieter Wuille)
Pull request description:
This updates our src/secp256k1 subtree to the latest upstream master. The changes include:
* The introduction of safegcd-based modular inverses, reducing ECDSA signing time by 25%-30% and ECDSA verification time by 15%-17%.
* [Original paper](https://gcd.cr.yp.to/papers.html) by Daniel J. Bernstein and Bo-Yin Yang
* [Implementation](https://github.com/bitcoin-core/secp256k1/pull/767) by Peter Dettman; [final](https://github.com/bitcoin-core/secp256k1/pull/831) version
* [Explanation](https://github.com/bitcoin-core/secp256k1/blob/master/doc/safegcd_implementation.md) of the algorithm using Python snippets
* [Analysis](https://github.com/sipa/safegcd-bounds) of the maximum number of iterations the algorithm needs
* [Formal proof in Coq](https://medium.com/blockstream/a-formal-proof-of-safegcd-bounds-695e1735a348) by Russell O'Connor, for a high-level equivalent algorithm
* Removal of libgmp as an (optional) dependency (which wasn't used in the Bitcoin Core build)
* CI changes (Travis -> Cirrus)
* Build system improvements
ACKs for top commit:
laanwj:
Tested ACK 5c7ee1b2da
Tree-SHA512: ad8ac3746264d279556a4aa7efdde3733e114fdba8856dd53218588521f04d83950366f5c1ea8fd56329b4c7fe08eedf8e206f8f26dbe3f0f81852e138655431
The Tor v2 addresses, left over from when Tor v2 was supported will be
unserialized as a dummy, invalid `::` (all zeros) IPv6 address. Remove
them so that they do not take up space in addrman.
The return type is already enforced to be void by the
ternary operator:
./test/fuzz/util.h:47:25: error: right operand to ? is void, but left operand is of type *OTHER_TYPE*
((i++ == call_index ? callables() : void()), ...);
^ ~~~~~~~~~~~ ~~~~~~
3737d35fee fuzz: Terminate immediately if a fuzzing harness ever tries to perform a DNS lookup (belts and suspenders) (practicalswift)
Pull request description:
Terminate immediately if a fuzzing harness tries to perform a DNS lookup (belt and suspenders).
Obviously this _should_ never happen, but if it _does_ happen we want immediate termination instead of a DNS lookup :)
ACKs for top commit:
MarcoFalke:
review ACK 3737d35fee
Tree-SHA512: 51cd2d32def7f9f052e02f99c354656af1f807cc9fdf592ab765e620bfe660f1ed26e0484763f94aba650424b44959eafaf352bfd0f81aa273e350510e97356e
62cb8d98d2 qt: Drop BitcoinGUI* WalletFrame data member (Hennadii Stepanov)
f73e5c972a qt: Move CreateWalletActivity connection from WalletFrame to BitcoinGUI (Hennadii Stepanov)
20e2e24e90 qt: Move WalletView connections from WalletFrame to BitcoinGUI (Hennadii Stepanov)
Pull request description:
This PR:
- implements an idea from https://github.com/bitcoin/bitcoin/pull/17937#issuecomment-575991765
- simplifies `WalletFrame` class interface
- as a side effect, removes `bitcoingui` -> `walletframe` -> `bitcoingui` circular dependency
- is an alternative to https://github.com/bitcoin/bitcoin/pull/17500
ACKs for top commit:
promag:
Tested ACK 62cb8d98d2 on macos 11.2.3 with depends build.
jarolrod:
ACK 62cb8d98d2
Tree-SHA512: 633b526a8499ba9ab4b16928daf4de4f6d610284bb9fa51891cad35300a03bde740df3466a71b46e87a62121330fcc9e606eac7666ea5e45fa6d5785b60dcbbd
fb1b1e0f3e qt: Save/restore column sizes of the tables in the Peers tab (Hennadii Stepanov)
Pull request description:
ACKs for top commit:
jonatack:
ACK fb1b1e0f3e code review, debug-built and tested
jarolrod:
ACK fb1b1e0f3e
Tree-SHA512: f93495ecd13e4202aba61b407fffbeec855f5b0c1cc027197c78edddd7d11c87ebdb0fcb1daac242f0407323b31f4e7e0313bd76113a5241e4c868a8829af20a
8b419b5163 qt: make console buttons look clickable (Jarol Rodriguez)
Pull request description:
On master, for macOS, the console buttons' hitboxes are quite small. This makes clicking on the button with your mouse a little more tedious than it should be. The Issue is related to recent versions of Qt (>5.9.8) not playing so nice on macOS when there are "incorrect" `width` and `height` values set for a `QPushButton` (here is another example: https://github.com/bitcoin-core/gui/pull/319#pullrequestreview-652907740).
This fixes this small hitbox issue by converting the buttons from `QPushButton` to `QToolButton`, which in turn makes the buttons look explicitly clickable. This approach was chosen as it helps us avoid having to play around with `width` and `height` values until we find values that play nice with macOS and look good on Linux & Windows. Also, `QToolButton` is an appropriate class for these buttons.
Per [Qt Docs](https://doc.qt.io/qt-5/qtoolbutton.html#details):
> A tool button is a special button that provides quick-access to specific commands or options. As opposed to a normal command button, a tool button usually doesn't show a text label, but shows an icon instead.
Since we are changing the type of the buttons, we need to change the respective actions connection logic in `rpcconsole`. Instead of plugging in `QToolButton`, we abstract it to the base class: `QAbstractButton`.
per [Qt Dev Notes](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Developer-Notes-for-Qt-Code#inherited-signals-and-slot)
> Use base class functions as this makes the code more general, e.g., use QAbstractButton::clicked instead of QPushButton::clicked.
While here, we also update the size of the icons to `22x22` to be consistent with other tool buttons.
**macOS: Master vs PR:**
| Master | PR |
| ----------- | ----------- |
|  |  |
**Linux: Master vs PR:**
| Master | PR |
| ----------- | ----------- |
|  |  |
ACKs for top commit:
hebasto:
ACK 8b419b5163, tested on Linux Mint 20.1 (Qt 5.12.8).
promag:
Tested ACK 8b419b5163 on macOS Big Sur M1, this drops only relevant usages to `flat` buttons.
Tree-SHA512: 3f3cdcbe83398136a1d1ee8fc2835be8681f2ed39e79db1e939cab6a00a779f528343d54992807a845cc84d9ef13591affb7a6dbca9e5753a2b8665b0af4d611
Reduce the condition `IsRFC4193() && !IsTor()` to `IsRFC4193()`. We know
that if `IsRFC4193()` is `true` then, for sure, the address is not Tor,
so `!IsTor()` is also `true`.
3d552b0d78 [doc] explain why CheckBlock() is called before AcceptBlock() (Sjors Provoost)
Pull request description:
Based on https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2019-February/016697.html and its PDF attachment.
ACKs for top commit:
MarcoFalke:
cr ACK 3d552b0d78
Tree-SHA512: d1ef39855317853e0e7e051ec6015054d0d227fcdf20281c2c1921056537f1f79044aa1bdd35f46475edd17596fbcae79aeb338c4865b1269a01b158f6cb2ac4
If all our high-bandwidth compact block serving peers (BIP 152) stall block
download, then we can be denied a block for (potentially) a long time. As
inbound connections are much more likely to be adversarial than outbound
connections, mitigate this risk by never removing our last outbound HB peer if
it would be replaced by an inbound.
2667366aaa tests: check derivation of P2TR (Pieter Wuille)
7cedafc541 Add tr() descriptor (derivation only, no signing) (Pieter Wuille)
90fcac365e Add TaprootBuilder class (Pieter Wuille)
5f6cc8daa8 Add XOnlyPubKey::CreateTapTweak (Pieter Wuille)
2fbfb1becb Make consensus checking of tweaks in pubkey.* Taproot-specific (Pieter Wuille)
a4bf84039c Separate WitnessV1Taproot variant in CTxDestination (Pieter Wuille)
41839bdb89 Avoid dependence on CTxDestination index order (Pieter Wuille)
31df02a070 Change Solver() output for WITNESS_V1_TAPROOT (Pieter Wuille)
4b1cc08f9f Make XOnlyPubKey act like byte container (Pieter Wuille)
Pull request description:
This is a subset of #21365, to aide review.
This adds support `tr(KEY)` or `tr(KEY,SCRIPT)` or `tr(KEY,{{S1,{{S2,S3},...}},...})` descriptors, describing Taproot outputs with specified internal key, and optionally any number of scripts, in nested groups of 2 inside `{`/`}` if there are more than one. While it permits importing `tr(KEY)`, anything beyond that is just laying foundations for more features later.
Missing:
* Signing support (see #21365)
* Support for more interesting scripts inside the tree (only `pk(KEY)` is supported for now). In particular, a multisig policy based on the new `OP_CHECKSIGADD` opcode would be very useful.
* Inferring `tr()` descriptors from outputs (given sufficient information).
* `getaddressinfo` support.
* MuSig support. Standardizing that is still an ongoing effort, and is generally kind of useless without corresponding PSBT support.
* Convenient ways of constructing descriptors without spendable internal key (especially ones that arent't trivially recognizable as such).
ACKs for top commit:
Sjors:
utACK 2667366 (based on https://github.com/bitcoin/bitcoin/pull/21365#issuecomment-846945215 review, plus the new functional test)
achow101:
Code Review ACK 2667366aaa
lsilva01:
Tested ACK 2667366aaa
meshcollider:
utACK 2667366aaa
Tree-SHA512: 61046fef22c561228338cb178422f0b782ef6587ec8208d3ce2bd07afcff29a664b54b35c6b01226eb70b6540b43f6dd245043d09aa6cb6db1381b6042667e75
5d82a57db4 contrib: remove torv2 seed nodes (Jon Atack)
5f7e086dac contrib: update generate-seeds.py to ignore torv2 addresses (Jon Atack)
8be56f0f8e p2p, refactor: extract OnionToString() from CNetAddr::ToStringIp() (Jon Atack)
5f9d3c09b4 p2p: remove torv2 from CNetAddr::ToStringIP() (Jon Atack)
3d39042144 p2p: remove torv2 in SetIP() and ADDR_TORV2_SIZE constant (Jon Atack)
cff5ec477a p2p: remove pre-addrv2 onions from SerializeV1Array() (Jon Atack)
4192a74413 p2p: ignore torv2-in-ipv6 addresses in SetLegacyIPv6() (Jon Atack)
1d631e956f p2p: remove BIP155Network::TORV2 from GetBIP155Network() (Jon Atack)
7d1769bc45 p2p: remove torv2 from SetNetFromBIP155Network() (Jon Atack)
eba9a94b9f fuzz: rename CNetAddr/CService deserialize targets (Jon Atack)
c56a1c9b18 p2p: drop onions from IsAddrV1Compatible(), no longer relay torv2 (Jon Atack)
f8e94002fc p2p: remove torv2/ADDR_TORV2_SIZE from SetTor() (Jon Atack)
0f1c58ae87 test: update feature_proxy to torv3 (Jon Atack)
Pull request description:
This patch removes support in Bitcoin Core for Tor v2 onions, which are already removed from the release of Tor 0.4.6.
- no longer serialize/deserialize and relay Tor v2 addresses
- ignore incoming Tor v2 addresses
- remove Tor v2 addresses from the addrman and peers.dat on node launch
- update generate-seeds.py to ignore Tor v2 addresses
- remove Tor v2 hard-coded seeds
Tested with tor-0.4.6.1-alpha (no v2 support) and 0.4.5.7 (v2 support). With the latest Tor (no v2 support), this removes all the warnings like those reported with current master in https://github.com/bitcoin/bitcoin/issues/21351
```
<bitcoind debug log>
Socks5() connect to […].onion:8333 failed: general failure
<tor log>
Invalid hostname [scrubbed]; rejecting
```
and the addrman no longer has Tor v2 addresses on launching bitcoind.
```rake
$ ./src/bitcoin-cli -addrinfo
{
"addresses_known": {
"ipv4": 44483,
"ipv6": 8467,
"torv2": 0,
"torv3": 2296,
"i2p": 6,
"total": 55252
}
}
```
After recompiling back to current master and restarting with either of the two Tor versions (0.4.5.7 or 0.4.6.1), -addrinfo initially returns 0 Tor v2 addresses and then begins finding them again.
Ran nodes on this patch over the past week on mainnet/testnet/signet/regtest after building with DEBUG_ADDRMAN.
Verified that this patch bootstraps an onlynet=onion node from the Tor v3 hardcoded fixed seeds on mainnet and testnet and connects to blocks and v3 onion peers: `rm ~/.bitcoin/testnet3/peers.dat ; ./src/bitcoind -testnet -dnsseed=0 -onlynet=onion`
Tested using `addnode`, `getaddednodeinfo`,`addpeeraddress`, `disconnectnode` and `-addrinfo` that a currently valid, connectable Tor v2 peer can no longer be added:
Thanks to Vasil Dimov, Carl Dong, and Wladimir J. van der Laan for their work on BIP155 and Tor v3 that got us here.
ACKs for top commit:
laanwj:
Code review ACK 5d82a57db4
Tree-SHA512: 590ff3d2f6ef682608596facb4b01f44fef69716d2ab3552ae1655aa225f4bf104f9ee08d6769abb9982a8031de93340df553279ce1f5023771f9f2b651178bb
f5ba424cd4 wallet: Add IsAddressUsed / SetAddressUsed methods (Russell Yanofsky)
62252c95e5 interfaces: Stop exposing wallet destdata to gui (Russell Yanofsky)
985430d9b2 test: Add gui test for wallet receive requests (Russell Yanofsky)
Pull request description:
Stop giving GUI access to destdata rows in database. Replace with narrow API just for saving and reading receive request information.
This simplifies code and should prevent the GUI from interfering with other destdata like address-used status. It also adds some more GUI test coverage.
There are no changes in behavior.
ACKs for top commit:
jarolrod:
tACK f5ba424cd4
laanwj:
Code review ACK f5ba424cd4
Tree-SHA512: 5423df4786e537a59013cb5bfb9e1bc29a7ca4b8835360c00cc2165a59f925fdc355907a4ceb8bca0285bb4946ba235bffa7645537a951ad03fd3b4cee17b6b0
a58868d201 build: Makes rcc output always deterministic (Hennadii Stepanov)
Pull request description:
The Qt Resource Compiler ([rcc](https://doc.qt.io/qt-5/rcc.html)) has a command-line option `--format-version` which has the [default value](https://code.qt.io/cgit/qt/qtbase.git/tree/src/tools/rcc/main.cpp?h=5.12.10#n172) 2.
The only difference from `--format-version 1` is adding a [last modified timestamp](https://code.qt.io/cgit/qt/qtbase.git/tree/src/tools/rcc/rcc.cpp?h=5.12.10#n207) to the output file ([credits](https://github.com/bitcoin/bitcoin/pull/21654#issuecomment-819198228) to **fanquake**). That, in turn, forces us to use `QT_RCC_SOURCE_DATE_OVERRIDE=1` to get deterministic builds (#13732).
This change makes rcc output always deterministic by using `--format-version 1` option that makes usage of the
`QT_RCC_SOURCE_DATE_OVERRIDE` needless.
---
Also it improves interaction with ccache:
On master (f6c44e999b):
```
$ make && make clean && ccache --zero-stats && make && ccache --show-stats
...
cache directory /home/hebasto/.ccache
primary config /home/hebasto/.ccache/ccache.conf
secondary config (readonly) /etc/ccache.conf
stats updated Sun Apr 11 15:45:43 2021
stats zeroed Sun Apr 11 15:45:05 2021
cache hit (direct) 638
cache hit (preprocessed) 0
cache miss 1
cache hit rate 99.84 %
called for link 10
cleanups performed 0
files in cache 20023
cache size 13.2 GB
max cache size 15.0 GB
```
The missed file is always `qt/libbitcoinqt_a-qrc_bitcoin_locale.o`.
With this PR:
```
$ make && make clean && ccache --zero-stats && make && ccache --show-stats
...
cache directory /home/hebasto/.ccache
primary config /home/hebasto/.ccache/ccache.conf
secondary config (readonly) /etc/ccache.conf
stats updated Sun Apr 11 15:28:46 2021
stats zeroed Sun Apr 11 15:28:21 2021
cache hit (direct) 639
cache hit (preprocessed) 0
cache miss 0
cache hit rate 100.00 %
called for link 10
cleanups performed 0
files in cache 20012
cache size 13.2 GB
max cache size 15.0 GB
```
ACKs for top commit:
fanquake:
ACK a58868d201
Tree-SHA512: 52f4a3267f41883d13025c0de79b6da22e92d60c729e01b986935c6812bbfe7fadc40b742bd715bfdf09df94af6838d4fbbe8208c6123f366108e38c8e1121c5
ab86ac7739 build, qt: Make QWindowsVistaStylePlugin available again (regression) (Hennadii Stepanov)
Pull request description:
This PR is similar to 1be8e0f238, and, actually, it is a #21376 follow up.
Required as in Qt 5.12.x style plugins are separated.
Fixes#22132.
Fixes https://github.com/bitcoin-core/gui/issues/303.
**Note for reviewers**. Besides visual changes in the GUI, you could compare the first dozen of lines in the `debug.log` file.
---
#### Guix build:
```
$ HOSTS=x86_64-w64-mingw32 contrib/guix/guix-build
$ find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 guix-build-ab86ac7739b2/output/dist-archive/SKIPATTEST.TAG
d3f05888c95d6cf6f4a0bac952d4c0bf1e271f49cb7ab92b08080b753957e5e5 guix-build-ab86ac7739b2/output/dist-archive/bitcoin-ab86ac7739b2.tar.gz
113bb86ffd2854e08d3c0c5cd3f9728f2e4fe6c7a1813bc1c4b73eb48ad6597f guix-build-ab86ac7739b2/output/x86_64-w64-mingw32/bitcoin-ab86ac7739b2-win-unsigned.tar.gz
3fdc99754d6f26d9655a308c8847076d8fd4db8d61f4f18d8ef545894a42c10d guix-build-ab86ac7739b2/output/x86_64-w64-mingw32/bitcoin-ab86ac7739b2-win64-debug.zip
932bb69e3eb1a617f9e337b83220c9d6a277bec421f4d78e32dffde5643eb00c guix-build-ab86ac7739b2/output/x86_64-w64-mingw32/bitcoin-ab86ac7739b2-win64-setup-unsigned.exe
852f4c03ad049fcd6bb2ce25b3bcf4e18b90d34f7232660024b341bd7c8dd710 guix-build-ab86ac7739b2/output/x86_64-w64-mingw32/bitcoin-ab86ac7739b2-win64.zip
c6a33a193a6b128e665198dde94758acab3a5ab7fefe0c3c6617eb98ad01693d guix-build-ab86ac7739b2/output/x86_64-w64-mingw32/inputs.SHA256SUMS
```
ACKs for top commit:
laanwj:
Concept and code review ACK ab86ac7739
fanquake:
ACK ab86ac7739
Tree-SHA512: 9f5498480379fad41de616da48331e123daf5b2294ca79c33ffea2b113b314634be9a2a70bf060cb5be1392a48e9d047891e1d22c129f408f1d76a0bc888441f
* Rename RewindBlockIndex -> NeedsRedownload (follow-up to commit
d831e711ca)
* Fix typos
* Inline comments about faking chain data to avoid duplicating them
Since headers-first syncing, we only ever request a block if we've already validated its headers.
Therefore QueuedBlock.fValidatedHeaders is always set to true. Remove it.
As proposed by @laanwj the error message is now prefixed with the
"timeout on transient error:" prefix, to explain why the error is
suddenly considered terminal.
Adds a new numeric `-rpcwaittimeout` that can be used to limit the
time we spend waiting on the RPC server to appear. This is used by
downstream projects to provide a bit of slack when `bitcoind`s RPC
interface is not available right away.
37371268d1 Mark `CheckTxInputs` `[[nodiscard]]` (out-param `txfee` only set if call is successful). Avoid UUM in fuzzing harness `coins_view`. (practicalswift)
Pull request description:
Mark `CheckTxInputs` `[[nodiscard]]` (out-param `txfee` only set if call is successful).
Avoid use of uninitialised memory (UUM) in fuzzing harness `coins_view`.
ACKs for top commit:
MarcoFalke:
review ACK 37371268d1
Tree-SHA512: edada5b2e80ce9ad3bd57b4c445bedefffa0a2d1cc880957d6848e4b7d9fc1ce036cd17f8b18bc03a36fbf84fc29c166cd6ac3dfbfe03e69d6fdbda13697754d
6c3fcd5591 test: remove BasicTestingSetup from util_threadnames unit tests (fanquake)
b53d3c1b1f test: remove BasicTestingSetup from uint256 unit tests (fanquake)
c0497a4928 test: remove BasicTestingSetup from torcontrol unit tests (fanquake)
ef8bb0473b test: remove BasicTestingSetup from sync unit tests (fanquake)
1aee83421f test: remove BasicTestingSetup from reverse_lock unit tests (fanquake)
57ba949ef5 test: remove BasicTestingSetup from policy_fee unit tests (fanquake)
3974c962b6 test: remove BasicTestingSetup from merkleblock tests (fanquake)
cd5bc4b470 test: remove BasicTestingSetup from hash unit tests (fanquake)
39cec22935 test: remove BasicTestingSetup from compilerbug unit tests (fanquake)
6d3b78c0e2 test: remove BasicTestingSetup from bswap unit tests (fanquake)
a13dc24831 test: remove BasicTestingSetup from bech32 unit tests (fanquake)
f4dcbe4498 test: remove BasicTestingSetup from base64 unit tests (fanquake)
fd144f6426 test: remove BasicTestingSetup from base32 unit tests (fanquake)
4c389ba04b test: remove BasicTestingSetup from arith_uint256 unit tests (fanquake)
05590651a0 test: remove BasicTestingSetup from amount unit tests (fanquake)
883a5c7d02 test: remove BasicTestingSetup from allocator unit tests (fanquake)
Pull request description:
* Less setup/overhead for tests that don't need it. Some naive bench-marking would suggest that a full `test_bitcoin` run is a few % faster after this change.
* Tests which don't need the BasicTestingSetup can't accidentally end up depending on it somehow.
* Already the case in at least the scheduler and block_filter tests.
This adds missing includes, but more significant is the removal of `setup_common.h` from tests where it isn't needed. This saves recompiling those tests when changes are made in the header.
ACKs for top commit:
practicalswift:
cr ACK 6c3fcd5591: patch looks correct
laanwj:
ACK 6c3fcd5591
Tree-SHA512: 69b891e2b4740402d62b86a4fc98c329a432d125971342a6f97334e166b3537ed3d4cdbb2531fa05c1feae32339c9fcb2dceda9afeeaed4edc70e8caa0962161
0680460041 qt: Translations update (Hennadii Stepanov)
Pull request description:
This is a pre-translation-string-freeze update. See [Release schedule for 22.0](https://github.com/bitcoin/bitcoin/issues/20851).
ACKs for top commit:
laanwj:
ACK 0680460041
Tree-SHA512: 65f11cc20e82459ca3484d4f377ff38be5ba31ba906abcb58d3e5ea56ee0eefd5e74c1ef1ea387833812bb2b9a02995dbeed7b45707e1d6db3f49e3b8055af6f
44d05d0a69 test: remove sanitizer suppression for nanobench (Martin Ankerl)
e3c866e3ca test: update nanobench from release 4.0.0 to 4.3.4 (Martin Ankerl)
Pull request description:
This updates the third-party library nanobench with the latest release. It contains mostly minor bugfixes, a new pyperf output format, ability to suppress warnings with environment variable `NANOBENCH_SUPPRESS_WARNINGS`. Full changelog:
v4.0.2
* Changed `doNotOptimizeAway` to what google benchmark is doing. The old code did not work on some machines.
* fix: display correct "total" value
* minor Documentation updates
v4.1.0
* Updated link to new pyperf home
* Adds ability to configure console output time unit
* Add support for environment variable `NANOBENCH_SUPPRESS_WARNINGS`
* Nanobench is now usable with CMake's FetchContent (see documentation: https://nanobench.ankerl.com/tutorial.html#cmake-integration)
v4.2.0
* Ability to store and later compare results added, through `pyperf`.
* See https://nanobench.ankerl.com/tutorial.html#pyperf-python-pyperf-module-output
* Added lots of build targets to travis, similar to bitcoin's build.
* Some minor API & documentation improvements
v4.3.0
* `ankerl::nanobench::Rng` can now return the state with `std::vector<uint64_t> Rng::state()`, and this can also be used to initialize the Rng.
v4.3.1
* Minor cmake improvements when integrationg as a third-party library: add alias `nanobench::nanobench`, default to C++17
v4.3.2
* Fixed a MSVC 2015 build problem
* updates license to 2021.
* build should now work with very old linux headers
* Also disable UBSAN (bitcoin needed to add a suppression)
v4.3.3
* Do not use locale-dependent `std::to_string`
v4.3.4
* Add missing sanitizer suppression to `rotl`
ACKs for top commit:
MarcoFalke:
review ACK 44d05d0a69
Tree-SHA512: 3291c85057720cfc84a44bfaa305a7d0df4dc35779169d20de73d32e40d4cdbf3f005bf343f79710eca517441de2459e8118c195c5f5136f99d1f50ebd5dfd08
e12f287498 net: cleanup newly added PeerManagerImpl::ProcessNewBlock (fanquake)
610151f5b0 validation: change ProcessNewBlock() to take a CBlock reference (fanquake)
Pull request description:
Addresses some [post-merge comments](https://github.com/bitcoin/bitcoin/pull/21713#pullrequestreview-638777410) from #21713. Also makes `ChainstateManager::ProcessNewBlock` take a const reference argument, as it [was asked](https://github.com/bitcoin/bitcoin/pull/21713#discussion_r615229548) why it was not the case in that PR.
ACKs for top commit:
jnewbery:
Code review ACK e12f287498
MarcoFalke:
review ACK e12f287498🚚
Tree-SHA512: 9c3e7353240c862d50bce2a0f58741c109dd628040b56ed46250103f8ebe9009238b131da710486791e28e3a83c985057b7be0a32aed1a929269b43097c7425b
7a799c9c2b index: refactor-only: Reuse CChain ref (Carl Dong)
db33cde80f index: Add chainstate member to BaseIndex (Carl Dong)
f4a47a1feb bench: Use existing chainman in AssembleBlock (Carl Dong)
91226eb917 bench: Use existing NodeContext in DuplicateInputs (Carl Dong)
e6b4aa6eb5 miner: Pass in chainman to RegenerateCommitments (Carl Dong)
9ecade1425 rest: Add GetChainman function and use it (Carl Dong)
fc1c282845 rpc/blockchain: Use existing blockman in gettxoutsetinfo (Carl Dong)
Pull request description:
Overall PR: #20158 (tree-wide: De-globalize ChainstateManager)
The first 2 commits are fixups addressing review for the last bundle: #21391
NEW note:
1. I have opened #21766 which keeps track of potential improvements where the flaws already existed before the de-globalization work, please post on that issue about these improvements, thanks!
Note to reviewers:
1. This bundle may _apparently_ introduce usage of `g_chainman` or `::Chain(state|)Active()` globals, but these are resolved later on in the overall PR. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
2. There may be seemingly obvious local references to `ChainstateManager` or other validation objects which are not being used in callers of the current function in question, this is done intentionally to **_keep each commit centered around one function/method_** to ease review and to make the overall change systematic. We don't assume anything about our callers. Rest assured that once we are considering that particular caller in later commits, we will use the obvious local references. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
3. When changing a function/method that has many callers (e.g. `LookupBlockIndex` with 55 callers), it is sometimes easier (and less error-prone) to use a scripted-diff. When doing so, there will be 3 commits in sequence so that every commit compiles like so:
1. Add `new_function`, make `old_function` a wrapper of `new_function`, divert all calls to `old_function` to `new_function` **in the local module only**
2. Scripted-diff to divert all calls to `old_function` to `new_function` **in the rest of the codebase**
3. Remove `old_function`
ACKs for top commit:
jarolrod:
ACK 7a799c9
ariard:
Code Review ACK 7a799c9
fjahr:
re-ACK 7a799c9c2b
MarcoFalke:
review ACK 7a799c9c2b🌠
ryanofsky:
Code review ACK 7a799c9c2b. Basically no change since last review except fixed rebase conflicts and a new comment about REST Ensure()
jamesob:
conditional ACK 7a799c9c2b ([`jamesob/ackr/21767.1.dongcarl.bundle_6_n_prune_g_chai`](https://github.com/jamesob/bitcoin/tree/ackr/21767.1.dongcarl.bundle_6_n_prune_g_chai))
Tree-SHA512: 531c00ddcb318817457db2812d9a9d930bc664e58e6f7f1c746350732b031dd624270bfa6b9f49d8056aeb6321d973f0e38e4ff914acd6768edd8602c017d10e
This updates the third-party library nanobench with the latest release. It contains mostly minor bugfixes, a new pyperf output format, ability to suppress warnings with environment variable `NANOBENCH_SUPPRESS_WARNINGS`. Full changelog:
v4.0.2
* Changed `doNotOptimizeAway` to what google benchmark is doing. The old code did not work on some machines.
* fix: display correct "total" value
* minor Documentation updates
v4.1.0
* Updated link to new pyperf home
* Adds ability to configure console output time unit
* Add support for environment variable `NANOBENCH_SUPPRESS_WARNINGS`
* Nanobench is now usable with CMake's FetchContent (see documentation: https://nanobench.ankerl.com/tutorial.html#cmake-integration)
v4.2.0
* Ability to store and later compare results added, through `pyperf`.
* See https://nanobench.ankerl.com/tutorial.html#pyperf-python-pyperf-module-output
* Added lots of build targets to travis, similar to bitcoin's build.
* Some minor API & documentation improvements
v4.3.0
* `ankerl::nanobench::Rng` can now return the state with `std::vector<uint64_t> Rng::state()`, and this can also be used to initialize the Rng.
v4.3.1
* Minor cmake improvements when integrationg as a third-party library: add alias `nanobench::nanobench`, default to C++17
v4.3.2
* Fixed a MSVC 2015 build problem
* updates license to 2021.
* build should now work with very old linux headers
* Also disable UBSAN (bitcoin needed to add a suppression)
v4.3.3
* Do not use locale-dependent `std::to_string`
v4.3.4
* Add missing sanitizer suppression to `rotl`
ffff0d0442 refactor: Switch serialize to uint8_t (1/n) (MarcoFalke)
Pull request description:
Replace `char` -> `uint8_t` in serialization where a sign doesn't make sense (char might be signed/unsigned).
ACKs for top commit:
practicalswift:
cr ACK ffff0d0442: patch looks correct and commit hash is ffffresh (was bbbbadass)
kristapsk:
ACK ffff0d0442
Tree-SHA512: cda682280c21d37cc3a6abd62569732079b31d18df3f157aa28bed80bd6f9f29a7db5c133b1f57b3a8f8d5ba181a76e473763c6e26a2df6d9244813f56f893ee
feb72e5432 scripted-diff: rename GetSystemTimeInSeconds to GetTimeSeconds (fanquake)
Pull request description:
This PR simply renames `GetSystemTimeInSeconds` to `GetTimeSeconds`, for uniformity amongst our time handling functions (`GetTimeMillis`, `GetTimeMicros`). I have a branch that does a chunk of `GetTime()` -> `GetSystemTimeInSeconds` (`GetTimeSeconds`) / `GetTime<T>` migration, so we can eventually remove the (2 year) deprecated `GetTime()`.
However, splitting this off and doing the renaming first while the number of `GetSystemTimeInSeconds` instances is small seems worthwhile.
ACKs for top commit:
practicalswift:
cr ACK feb72e5432: patch looks correct
promag:
Code review ACK feb72e5432.
Tree-SHA512: e2ac30be9cbcd77b70c9f74bef820b558945d0fcc6f3dc59fde68a18d08a7d36f42088b804ffe7c03478c8db048615b4c4aa65a3d8d9f5d717d59b58c99f1c54
38eb37c0bd qt, rpc: Do not accept command while executing another one (Hennadii Stepanov)
0c32b9c527 qt, rpc: Accept stop RPC even another command is executing (Hennadii Stepanov)
ccf790287c qt, rpc, refactor: Return early in RPCConsole::on_lineEdit_returnPressed (Hennadii Stepanov)
5b9c8c9cdd qt, rpc: Add "Executing…" message (Hennadii Stepanov)
Pull request description:
On master (3f512f3d56) it is possible to enter another command while the current command is still being executed. That makes a mess in the output.
With this PR:
Some previous context: https://github.com/bitcoin-core/gui/pull/59#issuecomment-715275185
---
It is still possible to enter and execute the `stop` command any time.
ACKs for top commit:
jarolrod:
ACK 38eb37c
promag:
Tested ACK 38eb37c0bd.
Tree-SHA512: 2b37a4b6838bf586b1b5c878192106721f713caeb6252514a6540356aab898986396e0777e73891d331b1be797a4926c20d3f9f38ba2c984ea90d55b0c34f664
0f3d955a38 qt: Make RPC console welcome message translation-friendly (Hennadii Stepanov)
Pull request description:
The best practice is do not split a translatable multi-line message into single lines. This helps translators to follow the context.
ACKs for top commit:
jarolrod:
re-ACK 0f3d955a38
Tree-SHA512: 30911ff3a972a7787804bb8b27d0b77bfff15939bb478c199261866bfb55d9acd12ab4d44b8b9fc1d4898222cabc4007cc897f9b65728924d121f31e914c44ac
d29ea72393 gui: Add access to the Peers tab from the network icon (Hennadii Stepanov)
Pull request description:
This PR add a small context menu to the network activity icon that provides an access to the Peers tab:
Closes#93.
ACKs for top commit:
Sjors:
re-ACK d29ea72393
kristapsk:
re-ACK d29ea72393
promag:
Code review ACK d29ea72393.
Tree-SHA512: dd871415fe514a19c6a22100d58f31954d9e55b80585d5a3f26e17a8d51dadf912441786fc0d23beabd812f1b501658fec1dbe345cd41beae5832a8eda890f77
SelectCoinsMinConf is a bit of a misnomer now since it really just does
all of the coin selection given some parameters. So rename this to
something less annoying to say and makes a bit more sense.
-BEGIN VERIFY SCRIPT-
sed -i 's/SelectCoinsMinConf/AttemptSelection/g' $(git grep -l SelectCoinsMinConf ./src)
-END VERIFY SCRIPT-
It's unnecessary to fill in the vin with dummy inputs, calculate the
fee, then fill in the vin with the actual inputs. Just fill the vin with
the actual inputs the first time.
- txNew nLockTime setting to txNew init
- FeeCalc to the fee estimation fetching
- setCoins to prior to SelectCoins
- nBytes to CalculateMaximumSignedTxSize call
- tx_sizes to CalculateMaximumSignedTxSize call
- coin_selection_params.m_avoid_partial_spends to params init
Ensuring that the recipients vector is not empty and that the amounts
are non-negative can be done in CreateTransaction rather than
CreateTransactionInternal. Additionally, these checks should happen as
soon as possible, so they are done at the beginning of
CreateTransaction.
nValue is the sum of the intended recipient amounts, so name it that for
clarity.
nValueToSelect is the coin selection target value, so name it
selection_target for clarity.
c7bd5842e4 MOVEONLY: CWallet transaction code out of wallet.cpp/.h (Russell Yanofsky)
Pull request description:
This commit just moves function without making any changes. It can be reviewed with `git log -p -n1 --color-moved=dimmed_zebra`
Motivation for this change is to make `wallet.cpp/h` less monolithic and start to make wallet transaction state tracking comprehensible so bugs in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking can be fixed safely without introducing new problems.
This moves wallet classes and methods that deal with transactions out of `wallet.cpp/.h` into better organized files:
- `transaction.cpp/.h` - CWalletTx and CMerkleTx class definitions
- `receive.cpp/.h` - functions checking received transactions and computing balances
- `spend.cpp/.h` - functions creating transactions and finding spendable coins
After #20773, when loading is separated from syncing it will also be possible to move more `wallet.cpp/.h` functions to:
- `sync.cpp/.h` - functions handling chain notifications and rescanning
This commit arranges `receive.cpp` and `spend.cpp` functions in dependency order so it's possible to skim `receive.cpp` and get an idea of how computing balances works, and skim `spend.cpp` and get an idea of how transactions are created, without having to jump all over `wallet.cpp` where functions are not in order and there is a lot of unrelated code.
Followup commit "refactor: Detach wallet transaction methods" in https://github.com/bitcoin/bitcoin/pull/21206 follows up this PR and tweaks function names and arguments to reflect new locations. The two commits are split into separate PRs because this commit is more work to maintain and less work to review, while the other commit is less work to maintain and more work to review, so hopefully this commit can be merged earlier.
ACKs for top commit:
Sjors:
re-utACK c7bd5842e4
fjahr:
utACK c7bd5842e4
promag:
Code review ACK c7bd5842e4, verified move only claim.
meshcollider:
Dimmed-zebra-check and functional test run ACK c7bd5842e4
Tree-SHA512: 4981de6911cb1196774db375494355cc9af59b52456129c002d264a77cd9ed6175f8ecbb6b2f492a59a4d5a0def21a39d96fa79c9f4d99be0992985f553be32f
4935ac583b qt: Improve GUI responsiveness (Hennadii Stepanov)
75850106ae qt, macos: Fix GUIUtil::PolishProgressDialog bug (Hennadii Stepanov)
Pull request description:
[`QProgressDialog`](https://doc.qt.io/qt-5/qprogressdialog.html) estimates the time the operation will take (based on time for steps), and only shows itself if that estimate is beyond [`minimumDuration`](https://doc.qt.io/qt-5/qprogressdialog.html#minimumDuration-prop).
The default `minimumDuration` value is [4 seconds](https://doc.qt.io/qt-5/qprogressdialog.html#details), and it could make users think that the GUI is frozen.
This PR sets `minimumDuration` to zero for all progress dialogs, that affects ones in the `WalletControllerActivity` class.
ACKs for top commit:
ryanofsky:
Code review ACK 4935ac583b. I'm not very familiar with this API but all the changes and explanations make sense and are very clear, and this seems like it should be an improvement.
promag:
Code review ACK 4935ac583b.
jarolrod:
ACK 4935ac583b
Tree-SHA512: 2ddd74e7fd87894d341d2439dbaa544d031a350f7f57d4c7e9fbba977dc24080fe60fd7a80a542b1647f1de9091d7fd04a36eab695088d4d75fb836548e99b5f
c231254a65 qt: Make TransactionView aware of runtime palette change (Hennadii Stepanov)
2b622d4ace qt: Make CoinControlDialog aware of runtime palette change (Hennadii Stepanov)
97a6b5e06a qt: Make OverviewPage aware of runtime palette change (Hennadii Stepanov)
d05f1b278d qt: Make UnitDisplayStatusBarControl aware of runtime palette change (Hennadii Stepanov)
6b2ce65392 qt: Replace base class of ClickableLabel with ThemedLabel (Hennadii Stepanov)
ff530a2093 qt: Use GUIUtil::ThemedLabel class (Hennadii Stepanov)
d99ef327a8 qt: Add GUIUtil::ThemedLabel class (Hennadii Stepanov)
c054720e08 qt: Make SignVerifyMessageDialog aware of runtime palette change (Hennadii Stepanov)
0dcc3fac43 qt: Make SendCoinsEntry aware of runtime palette change (Hennadii Stepanov)
fa18d28e12 qt: Make RPCConsole aware of runtime palette change (Hennadii Stepanov)
f1083826e3 qt: Make BitcoinGUI aware of runtime palette change (Hennadii Stepanov)
ce17861dc4 qt: Make PlatformStyle aware of runtime palette change (Hennadii Stepanov)
Pull request description:
On macOS switching appearance (Light -> Dark or Dark -> Light) when Bitcoin Core is running makes the GUI pretty unusable.
This bug is especially important when a user chose the "Auto" mode to adjust appearance automatically.
This PR fixes Bitcoin Core behavior.
This is an alternative to #268.
ACKs for top commit:
Sjors:
tACK c231254a65 on macOS 11.4
goums:
ACK c231254a65
promag:
Tested ACK c231254a65 on macOS Big Sur arm64.
jarolrod:
tACK c231254a65
Tree-SHA512: 122dda3e4c9703f68cec60613c536ca59d04c93f2c03398559f2361b8d279ae534800e8e677d94a33e10e769d00be54295a704e98afa2e986a06146b9f164854
`CAddrMan` uses `std::map` internally even though it does not require
that the map's elements are sorted. `std::map`'s access time is
`O(log(map size))`. `std::unordered_map` is more suitable as it has a
`O(1)` access time.
This patch lowers the execution times of `CAddrMan`'s methods as follows
(as per `src/bench/addrman.cpp`):
```
AddrMan::Add(): -3.5%
AddrMan::GetAddr(): -76%
AddrMan::Good(): -0.38%
AddrMan::Select(): -45%
```
7d07192dde Add src/qt/android/.gitignore (Hennadii Stepanov)
Pull request description:
This PR makes `git` ignore files created by `make apk`.
ACKs for top commit:
icota:
ACK 7d07192dde
Tree-SHA512: 4be20bd84830217a10d8ea7634799e71ed50be73f4f60c91c56311a2c95b22ff1f28d3b7bc077f1417318bb75e446e3fc3bdbf9dbc037b4cbc8428f0875f2c77
13650fe2e5 [policy] detect unsorted packages (glozow)
9ef643e21b [doc] add release note for package testmempoolaccept (glozow)
c4259f4b7e [test] functional test for packages in RPCs (glozow)
9ede34a6f2 [rpc] allow multiple txns in testmempoolaccept (glozow)
ae8e6df709 [policy] limit package sizes (glozow)
c9e1a26d1f [fuzz] add ProcessNewPackage call in tx_pool fuzzer (glozow)
363e3d916c [test] unit tests for ProcessNewPackage (glozow)
cd9a11ac96 [test] make submit optional in CreateValidMempoolTransaction (glozow)
2ef187941d [validation] package validation for test accepts (glozow)
578148ded6 [validation] explicit Success/Failure ctors for MempoolAcceptResult (glozow)
b88d77aec5 [policy] Define packages (glozow)
249f43f3cc [refactor] add option to disable RBF (glozow)
897e348f59 [coins/mempool] extend CCoinsViewMemPool to track temporary coins (glozow)
42cf8b25df [validation] make CheckSequenceLocks context-free (glozow)
Pull request description:
This PR enables validation dry-runs of packages through the `testmempoolaccept` RPC. The expectation is that the results returned from `testmempoolaccept` are what you'd get from test-then-submitting each transaction individually, in that order (this means the package is expected to be sorted in topological order, for now at least). The validation is also atomic: in the case of failure, it immediately halts and may return "unfinished" `MempoolAcceptResult`s for transactions that weren't fully validated. The API for 1 transaction stays the same.
**Motivation:**
- This allows you to test validity for transaction chains (e.g. with multiple spending paths and where you don't want to broadcast yet); closes#18480.
- It's also a first step towards package validation in a minimally invasive way.
- The RPC commit happens to close#21074 by clarifying the "allowed" key.
There are a few added restrictions on the packages, mostly to simplify the logic for areas that aren't critical to main package use cases:
- No package can have conflicts, i.e. none of them can spend the same inputs, even if it would be a valid BIP125 replacement.
- The package cannot conflict with the mempool, i.e. RBF is disabled.
- The total count of the package cannot exceed 25 (the default descendant count limit), and total size cannot exceed 101KvB (the default descendant size limit).
If you're looking for review comments and github isn't loading them, I have a gist compiling some topics of discussion [here](https://gist.github.com/glozow/c3acaf161c95bba491fce31585b2aaf7)
ACKs for top commit:
laanwj:
Code review re-ACK 13650fe2e5
jnewbery:
Code review ACK 13650fe2e5
ariard:
ACK 13650fe
Tree-SHA512: 8c5cbfa91a6c714e1c8710bb281d5ff1c5af36741872a7c5df6b24874d6272b4a09f816cb8a4c7de33ef8e1c2a2c252c0df5105b7802f70bc6ff821ed7cc1a2f
Pass in chainman instead of prev_block so that we can enforce the
block.hashPrevBlock refers to prev_block invariant in the function
itself.
We should probably rethink BlockAssembler's API and somehow include
commitment regeneration functionality in there. Something like a variant
of CreateNewBlock that takes in a std::vector<TxRef> and return a CBlock
instead of CBlockTemplate. That could avoid reaching for
LookupBlockIndex at all.
This is not the cleanest change but:
1. It fixes the erroneous use of RPC's Ensure*() in rest.cpp, which
cause crashes in REST contexts.
RPC code wraps all calls in a try/except, REST code does not.
Ensure*(), being part of RPC, expects that its throw's will get
caught by a try/except. But if you use Ensure*() in REST code, since
it doesn't have a try/except wrap, a crash will happen.
2. It is consistent with other functions like GetMemPool.
Someone can probably make this a bit prettier.
8caf60dbbe move-only: Group and re-order CAddrMan members by access type (Hennadii Stepanov)
5cd7f8abe3 refactor: Do not expose CAddrMan members as protected without need (Hennadii Stepanov)
Pull request description:
This PR is split from #19238 as all of its commits are trivial to review.
The last commit is easy to review with `git diff --color-moved=dimmed-zebra`.
Addressed the following comments from #19238:
- 130b82ff35 (r550865131)
> Can you consolidate all the private members and protected members to be next to each other? Multiple private and protected access specifiers make this harder to read than is necessary.
- 130b82ff35 (r557271783)
> Yeah, class declaration is easier to read if there is just one instance of `public:`, `protected:` and `private:` (in that order).
ACKs for top commit:
jnewbery:
ACK 8caf60dbbe
laanwj:
Code review ACK 8caf60dbbe
jarolrod:
ACK 8caf60dbbe
vasild:
ACK 8caf60dbbe
Tree-SHA512: e6127fc658da7876e36f22e2fae162dc19502ed7f8e931fdebc827dabd627e5346c6fbe6f6d0cd27fd3e5c96690ff35022ff6b48f2747b748ebd66a45c851c2b
df4c81fda4 English translations update (Hennadii Stepanov)
bfb53ddda9 scripted-diff: Fix ellipsis after pr20773 (Hennadii Stepanov)
Pull request description:
Update for Transifex.
After changing translator comments in #332 this update will show if Transifex triggers strings to be re-translated.
ACKs for top commit:
laanwj:
ACK df4c81fda4
jarolrod:
ACK df4c81fda4
Tree-SHA512: 1e54812bc04db6ae39e0b4d735b220ed8730a9941b17a0a2d09e21bcdd08e829adba86c35cf43c9be5e492ccb13e53a90149fcd7d6c0f5fdd022b978a1ff785c
cb7eba2a57 build: Use Qt archive of the same version as the compiled binaries (Hennadii Stepanov)
Pull request description:
This PR fixes broken Android APK build when the `depends/sources` directory contains Qt source archives of different versions (e.g., Qt version update [pull request](https://github.com/bitcoin/bitcoin/pull/22054) in CI with the cached `depends/sources` directory).
This is an alternative to #22058.
ACKs for top commit:
MarcoFalke:
review ACK cb7eba2a57
laanwj:
Code review ACK cb7eba2a57
Tree-SHA512: cf63a9809fba5cb13719d7e7bb5afc718a2cff5233b0670d30d30a0018d91278fcfc2a1b9ae8b84e8e3a52c95157bc465603cc754bb8a9d1a3d62415f01ad70f
