Separate out the management of chain-agnostic block metadata from any given
CChainState instance. This allows us to avoid duplicating data like
`mapBlockIndex` unnecessarily for multiple chainstates.
This also adds a CChainState constructor that accepts and sets m_blockman.
Ultimately this reference will point to a BlockMan instance that
is shared across CChainStates.
This commit can be decomposed into smaller commits if necessary.
976b034b13 [build]: use #if HAVE_SYSTEM instead of defined(HAVE_SYSTEM) (Sjors Provoost)
Pull request description:
It seems that `AC_DEFINE([HAVE_SYSTEM], [HAVE_STD__SYSTEM || HAVE_WSYSTEM]` causes `HAVE_SYSTEM` to always be defined, so we need to use `#if HAVE_SYSTEM` instead of `#if defined(HAVE_SYSTEM)`.
Followup for #15457, can be tested with #12557.
ACKs for top commit:
dongcarl:
ACK 976b034b13.
promag:
ACK 976b034b13.
fanquake:
ACK 976b034b13
Tree-SHA512: b8cdd04c2ec399fd15638aef5d75ea0886ec1572d3cf4fcea27c193e1e6390344315908262cad8981a9b0a905ab9520619ce2ffe9a717f4ee6bfa8b028ebbdc6
91cc18f602 [docs] Add release notes for PR 15427 (John Newbery)
3b11420b3c [RPC] add new utxoupdatepsbt arguments to the CRPCCommand and CPRCConvertParam tables (John Newbery)
Pull request description:
The new `descriptors` argument was not added to the CRPCCommand and CPRCCvertParam tables, meaning that it couldn't be used with bitcoin-cli or named arguments.
Before this PR:
```
> bitcoin-cli utxoupdatepsbt 'cHNidP8BAFMCAAAAAYCdwVRx2X3o4KHx5tAMsN1ddp51MbfWsietjfMbl5HtAAAAAAD/////AQDh9QUAAAAAF6kUW+rtEOi4nk9rpw2F5XZl1dd8ehGHAAAAAAAAAA==' "[{\"desc\":\"sh(wpkh([bd50871a/0h/0h/0h]03895c66337b38699bfafff1084ad35bc347fac4f4e5e5fe5eb7dd81155280db53))\"}]"
error code: -3
error message:
Expected type array, got string
> bitcoin-cli --named utxoupdatepsbt psbt='cHNidP8BAFMCAAAAAYCdwVRx2X3o4KHx5tAMsN1ddp51MbfWsietjfMbl5HtAAAAAAD/////AQDh9QUAAAAAF6kUW+rtEOi4nk9rpw2F5XZl1dd8ehGHAAAAAAAAAA==' descriptors="[{\"desc\":\"sh(wpkh([bd50871a/0h/0h/0h]03895c66337b38699bfafff1084ad35bc347fac4f4e5e5fe5eb7dd81155280db53))\"}]"
error code: -8
error message:
Unknown named parameter descriptors
```
After this PR:
```
bitcoin-cli utxoupdatepsbt 'cHNidP8BAFMCAAAAAYCdwVRx2X3o4KHx5tAMsN1ddp51MbfWsietjfMbl5HtAAAAAAD/////AQDh9QUAAAAAF6kUW+rtEOi4nk9rpw2F5XZl1dd8ehGHAAAAAAAAAA==' "[{\"desc\":\"sh(wpkh([bd50871a/0h/0h/0h]03895c66337b38699bfafff1084ad35bc347fac4f4e5e5fe5eb7dd81155280db53))\"}]"
cHNidP8BAFMCAAAAAYCdwVRx2X3o4KHx5tAMsN1ddp51MbfWsietjfMbl5HtAAAAAAD/////AQDh9QUAAAAAF6kUW+rtEOi4nk9rpw2F5XZl1dd8ehGHAAAAAAAAAA==
bitcoin-cli --named utxoupdatepsbt psbt='cHNidP8BAFMCAAAAAYCdwVRx2X3o4KHx5tAMsN1ddp51MbfWsietjfMbl5HtAAAAAAD/////AQDh9QUAAAAAF6kUW+rtEOi4nk9rpw2F5XZl1dd8ehGHAAAAAAAAAA==' descriptors="[{\"desc\":\"sh(wpkh([bd50871a/0h/0h/0h]03895c66337b38699bfafff1084ad35bc347fac4f4e5e5fe5eb7dd81155280db53))\"}]"
cHNidP8BAFMCAAAAAYCdwVRx2X3o4KHx5tAMsN1ddp51MbfWsietjfMbl5HtAAAAAAD/////AQDh9QUAAAAAF6kUW+rtEOi4nk9rpw2F5XZl1dd8ehGHAAAAAAAAAA==
```
ACKs for top commit:
promag:
ACK 91cc18f.
fanquake:
re-ACK 91cc18f602
Tree-SHA512: 279b2339a5cac17e363002e4ab743e251d6757c904c89f1970575bdce18d4f63d5e13507e171bf2bdc1bf6dd457db345a4b11b15d4ff71b96c2fedc4ffe52b23
f874e14cd3 [build]: check std::system for -[alert|block|wallet]notify (Sjors Provoost)
cc3ad56ff2 [build] MSVC: set HAVE_SYSTEM for desktop apps (Sjors Provoost)
c1c91bb78d [build] detect std::system or ::wsystem (Sjors Provoost)
Pull request description:
Platforms such as iOs and Universal Windows Platform do not support launching a process through system().
ACKs for top commit:
laanwj:
code review ACK f874e14cd3
Tree-SHA512: 16bb4a8fa1896046ccb22a46c8985e1aa45f5b11ecf5539eb2299e9a58f1a5b085c0c12cb6939c7493d93abce7e84fadcbfc73374c887db63da6d00c08aa476d
1ac454a384 Enable ShellCheck rules (Hennadii Stepanov)
Pull request description:
Enable some simple ShellCheck rules.
Note for reviewers: `bash` and `shellcheck` on macOS are different from ones on Ubuntu.
For local tests the latest `shellcheck` version 0.6.0 should be used (see #15166).
ACKs for top commit:
practicalswift:
utACK 1ac454a384
dongcarl:
utACK 1ac454a
fanquake:
ACK 1ac454a384
Tree-SHA512: 8d0a3a5c09fe1a0c22120178f5e6b80f81f746f8c3356b7701ff301c117acb2edea8fe08f08fb54ed73f94b1617515fb239fa28e7ab4121f74872e6494b6f20e
The new `descriptors` argument needs to be added to the Command and
ConvertParams tables to by usable as a named argument and by
bitcoin-cli.
Also update the test to use named arguments to test this.
3d60a03a7c bench: Move generated data to a dedicated translation unit (João Barbosa)
Pull request description:
With this change multiple benchmarks can use the same data without incurring in a bigger binary.
ACKs for top commit:
laanwj:
code review ACK 3d60a03a7c
Tree-SHA512: 8903bb09e4327c88e585a09bc7df1cbdfc18ebdc5d9c86bf3d6d9252a05eaf18b14ecd2bafdacd82f05a659e4b35ecd301c36011c97f7bf89302793165b00fdc
fab0c820fa rpc: Clarify that block count means height excl genesis (MarcoFalke)
Pull request description:
There is a common misconception that the block count returned by the blockchain rpcs includes the genesis block. See for example the discussion in https://github.com/bitcoin/bitcoin/pull/16292#issuecomment-506303256.
However, it really returns the height, which is `0` for the genesis block.
So clarify that and also remove the misleading "longest blockchain" comment.
Finally, fix the wallet test that incorrectly used this rpc.
ACKs for top commit:
instagibbs:
utACK fab0c820fa
promag:
ACK fab0c82, sorry for the misconception.
Tree-SHA512: 0d087cbb628d3866352bca6420402f392e6a997e579941701a408a7fca355d84645045661f39b022e4479cc07f85a6cddaa9095b6fd9911b245692482420a5e4
8a6810d0d2 Add a 'logpath' field to getrpcinfo (darosior)
Pull request description:
as discussed in #15438
ACKs for top commit:
laanwj:
Tested ACK 8a6810d0d2
Tree-SHA512: 752c7d90f670677c8144efb338c5c97c2264f85f1e65e031fd5a44f04230b6eafbabd0f634db263eb42c25642ecc1c4b1b602d4735e3fab07ec00b566134ddab
0f459d868d fix an undefined behavior in uint::SetHex (Kaz Wesley)
Pull request description:
Decrementing psz beyond the beginning of the string is UB, even though
the out-of-bounds pointer is never dereferenced.
I don't think any clang sanitizer covers this, so I don't see any way a test could catch the original behavior.
ACKs for top commit:
promag:
utACK 0f459d8.
l2a5b1:
utACK 0f459d868d
Tree-SHA512: 388223254ea6e955f643d2ebdf74d15a3d494e9f0597d9f05987ebb708d7a1cc06ce64bd25d447d75b5f5561bdae9630dcf25adb7bd75f7a382298b95d127162
faa1e0fb17 qt: test: Create at most one testing setup (MarcoFalke)
Pull request description:
It is assumed that ideally only one BasicTestingSetup exists at any point in time for each process (due to use of globals).
This assumption is violated in the GUI tests, as a testing setup is created as the first step of the `main` function and then (sometimes) another one for the following test cases.
So, the gui tests create two testing setups:
* `BasicTestingSetup` in `main` (added in fa4a04a5a9)
* a testing setup for individual test cases
Avoid that by destructing the testing setup in main after creation and then move the explicit `ECC_Stop` to the only places where it is needed (before and after `apptests`).
ACKs for top commit:
laanwj:
code review ACK faa1e0fb17
Tree-SHA512: b8edceb7e2a8749e1de3ea80bc20b6fb7d4390bf366bb9817206ada3dc8669a91416f4803c22a0e6c636c514e0c858dcfe04523221f8851b10deaf472f107d82
f53a70ce95 Improve documentation of memory_cleanse() (Tim Ruffing)
cac30a436c Clean up logic in memory_cleanse() for MSVC (Tim Ruffing)
Pull request description:
When working on https://github.com/bitcoin-core/secp256k1/issues/185, I noticed that the logic in memory_cleanse(), which is supposed to clear memory securely, is weird on MSVC. While it's correct, it's at least a code smell because the code clears the memory twice on MSVC. This weirdness was introduced by #11558.
This PR fixes the logic on MSVC and also improves the docs around this function. Best reviewed in individual commits, see the commit messages for more rationale. The second commit touches only comments.
ACKs for top commit:
practicalswift:
utACK f53a70ce95 :-)
laanwj:
code review ACK f53a70ce95
Tree-SHA512: 1c2fd98ae62b34b3e6e59d1178b293af969a9e06cbb7df02a699ce8802f145a336f72edb178c520e3ecec81f7e8083828f90a5ba6367d966a2c7d7c0dd6c0475
3b9bf0eb0e rpc: Allow shutdown while in generateblocks (Patrick Strateman)
Pull request description:
By checking the shutdown flag every loop we can use the entire 32 bit nonce space instead of breaking every 16 bits to check the flag.
This is possible now because the shutdown flag is an atomic where before it was controlled by a condition variable and lock.
ACKs for top commit:
kallewoof:
Re-ACK 3b9bf0e
Tree-SHA512: d0664201a55215130c2e9199a31fb81361daf4102a65cb3418984fd61cb98bfb9136d9ee8d23a85d57e50051f9bb0059bd71fe0488a17f63c38ea5caa6004504
01174596e6 signrawtransactionwithkey: report error when missing redeemScript/witnessScript param (Anthony Towns)
Pull request description:
Adding support for "witnessScript" as an alternative to "redeemScript" when using "signrawtransactionwithkey" meant that the `RPCTypeCheckObj()` call in `SignTransaction` can't error out just because either parameter is missing -- it's only a problem if both are missing, which isn't a state `RPCTypeCheckObj()` tests for. This results in the regression described in #16249. This patch adds some code to test for this case and give a similar error, namely:
error code: -8
error message:
Missing redeemScript/witnessScript
Fixes: #16249
ACKs for top commit:
meshcollider:
utACK 01174596e6
promag:
ACK 01174596e. Could also write test without `dict`/`del`:
Tree-SHA512: cf51346b7dea551b7f18f2a93c2a336a293b2535c62c03a5263cd2be8c58cf0cc302891da659c167e88ad1a68a756472c3c07e99f71627c61d32886fc5a3a353
26fe9b9909 Add support for descriptors to utxoupdatepsbt (Pieter Wuille)
3135c1a2d2 Abstract out UpdatePSBTOutput from FillPSBT (Pieter Wuille)
fb90ec3c33 Abstract out EvalDescriptorStringOrObject from scantxoutset (Pieter Wuille)
eaf4f88734 Abstract out IsSegWitOutput from utxoupdatepsbt (Pieter Wuille)
Pull request description:
This adds a descriptors argument to the `utxoupdatepsbt` RPC. This means:
* Input and output scripts and keys will be filled in when known.
* P2SH-witness inputs will be filled in from the UTXO set when a descriptor is provided that shows they're spending segwit outputs.
This also moves some (newly) shared code to separate functions: `UpdatePSBTOutput` (an analogue to `SignPSBTInput`), `IsSegWitOutput`, and `EvalDescriptorStringOrObject` (implementing the string or object notation parsing used in `scantxoutset`).
ACKs for top commit:
jnewbery:
utACK 26fe9b9909
laanwj:
utACK 26fe9b9909 (will hold merging until response to promag's comments)
promag:
ACK 26fe9b9, checked refactors and tests look comprehensive. Still missing a release note but can be added later.
Tree-SHA512: 1d833b7351b59d6c5ded6da399ff371a8a2a6ad04c0a8f90e6e46105dc737fa6f2740b1e5340280d59e01f42896c40b720c042f44417e38dfbee6477b894b245
fa2b083c3f [test] Add test to check mempool consistency in case of reorgs (MarcoFalke)
fabeb1f613 validation: Add missing mempool locks (MarcoFalke)
fa0c9dbf91 txpool: Make nTransactionsUpdated atomic (MarcoFalke)
Pull request description:
Take the mempool read lock during reorgs, so that we don't accidentally read an inconsistent mempool.
ACKs for top commit:
laanwj:
code review ACK fa2b083c3f
ryanofsky:
utACK fa2b083c3f [EDIT: was ~e284e422e75189794e24fe482819d8b1407857c3~, from bad copy and paste]. Changes since last review: rebase after #15976, adding vTxHashes lock annotation, adding new commit dropping mempool lock for nTransactionsUpdated and making it atomic to avoid deadlock between mempool lock and g_best_block_mutex
Tree-SHA512: cfe7777993589087753e000e3736d79d320dca412383fb77b56bef8946a04049722bf888c11b6f722adf677165185c7e58b4a269f7c5fa25e84dda375f6c8a7d
d9753383b9 addrdb: Remove temporary files created in SerializeFileDB. Fixes non-determinism in unit tests. (practicalswift)
Pull request description:
Remove temporary files created in `SerializeFileDB` in case of errors.
_Edit: Previously this was hit non-deterministically from the tests: that is no longer the case but the cleanup issue remains :-)_
ACKs for top commit:
laanwj:
code-review ACK d9753383b9
Tree-SHA512: e72b74b8de411f433bd8bb354cacae07ab75a240db6232bc6a37802ccd8086bff5275ce3d196ddde033d8ab9e2794bb8f60eb83554af7ec2e9f91d6186cb4647
806b0052c3 [wallet] abort when attempting to fund a transaction above maxtxfee (Sjors Provoost)
Pull request description:
`FundTransaction` calls `GetMinimumFee` which, when the fee rate is absurdly high, quietly reduces the fee to `-maxtxfee`.
Becaue an absurdly high fee rate is usually the result of a fat finger, aborting seems safer behavior.
Before:
```
bitcoin-cli walletcreatefundedpsbt '[]' '[{"tb1q...": 0.01}]' 0 '{"feeRate": 10}' true
{
"psbt": "cHNidP8...gAA=",
"fee": 0.10000000,
"changepos": 1
}
```
After:
```
bitcoin-cli walletcreatefundedpsbt '[]' '[{"tb1q...": 0.01}]' 0 '{"feeRate": 10}' true
error code: -25
error message:
Fee exceeds maximum configured by -maxtxfee
```
QT still checks the max fee rate as expected:
<img width="566" alt="Schermafbeelding 2019-06-20 om 19 52 00" src="https://user-images.githubusercontent.com/10217/59888424-a2aa7100-9395-11e9-8ae6-8a3c1f7de585.png">
ACKs for top commit:
laanwj:
Code review ACK 806b0052c3
Tree-SHA512: bee95811711cdab100b614d2347921407af3b400aea613ca156953ed3f60b924ad29a1d335bd0e240c0b7c0fbb360226bab03294d226a5560cdf2a3f21e6d406
So far, the documentation of memory_cleanse() is a verbatim copy of
the commit message in BoringSSL, where this code was originally
written. However, our code evolved since then, and the commit message
is not particularly helpful in the code but is rather of historical
interested in BoringSSL only.
This commit improves improves the comments around memory_cleanse()
and gives a better rationale for the method that we use. This commit
touches only comments.
FundTransaction calls GetMinimumFee which, when the fee rate is absurdly high, quietly reduced the fee to -maxtxfee. Becaue an absurdly high fee rate is usually the result of a fat finger, aborting seems safer behavior.
fa69c3e6ca util: Explain why the path is cached (MarcoFalke)
Pull request description:
The rationale for caching the datadir is given as
```
// This can be called during exceptions by LogPrintf(), so we cache the
// value so we don't have to do memory allocations after that.
```
Since 8c2d695c4a, the debug log location is actually cached itself in `m_file_path`.
So explain that the caching is now only used to guard against disk access on each call. (See also #16255)
ACKs for top commit:
promag:
ACK fa69c3e6ca.
laanwj:
ACK fa69c3e6ca
ryanofsky:
utACK fa69c3e6ca. Good cleanup. Previous comment was confusing, and definitely not helpful if outdated.
Tree-SHA512: 02108c90026d6d7c02843aaf59a06b4e1fa63d5d4378bb7760f50767efc340dc94c259bf7afb32fa4d47952b48a4e91798d1e0ddc1b051d770405e078636793a
9a841696c1 tests: Reduce compilation time and unneccessary recompiles by removing unused includes in tests (practicalswift)
Pull request description:
Reduce compilation time and unneccessary recompiles by removing unused includes in tests.
A subset of #16273 ("refactor: Reduce total compilation time by 2% and avoid unnecessary recompiles by removing unused includes") as requested by MarcoFalke in https://github.com/bitcoin/bitcoin/pull/16273#issuecomment-505022643.
ACKs for top commit:
Sjors:
ACK 9a84169 on macOS 10.14.5 (I rebased on #16289)
Tree-SHA512: bcb6ecffef689a9839bee1a5cb93abe83db1f30819a54226c5630fee456b5a5d187507d06861454adfda939c3556a975113f97662e415cb47fa0327ea4fd09fb
f466c4ce84 Add missing ECC_Stop(); in GUI rpcnestedtests.cpp (Jonas Schnelli)
Pull request description:
Fixes#16288
Was probably missing in #7783
ACKs for top commit:
Sjors:
ACK f466c4c. Tested by comparing `make check` on master and this PR with macOS 10.14.5. I also tried with and without `--enable-debug` / `--without-gui`.
fanquake:
ACK f466c4ce84. Tested running `make check` on macOS.
Tree-SHA512: 648e10c2e35bd01fb92e63709169a6c185ac4b62c69af0109d2cd2d7db47e56ae804c788f9a1a1845746f818764799732f9e58e9dbfca3bffeea8f14683c8c7f
9824a0d6e9 Remove extra CBlockIndex declaration (RJ Rybarczyk)
Pull request description:
Remove duplicate `class CBlockIndex;` declaration.
ACKs for top commit:
promag:
ACK 9824a0d. Is this a random finding or you have searched for more similar cases?
practicalswift:
utACK 9824a0d6e9
fanquake:
ACK 9824a0d6e9
Tree-SHA512: aaf88450f53cb8859778102fe971b1121808819c04e64802e5a5cf47bf1403b42531361c52b097b41b905f9fa1bb7acc82b446cfa659c6ac41d00fab29e114e4
fabc57e07d test: Log to debug.log in all tests (MarcoFalke)
fa4a04a5a9 test: use common setup in gui tests (MarcoFalke)
fad3d2a624 test: Create data dir in BasicTestingSetup (MarcoFalke)
Pull request description:
This makes it easier to debug a frozen test or a test that failed. To debug a failed test, remove the line `fs::remove_all(m_path_root);`.
The pull is done in three commits:
* Create a datadir for every unit test once (and only once). This requires the `SetDataDir` function to go away.
* Use the common setup in the gui unit tests. Some of those tests are testing the init sequence, so we'd have to undo some of what the testing setup did.
* Log to the debug.log in all tests
ACKs for top commit:
laanwj:
ACK fabc57e07d
Tree-SHA512: 73444210b88172669e2cd22c2703a1e30e105185d2d5f03decbdedcfd09c64ed208d3716c59c8bebb0e44214cee5c8095e3e995d049e1572ee98f1017e413665
dddd9270f8 net: Document what happens to getdata of unknonw type (MarcoFalke)
Pull request description:
Any getdata of unknown type will never be processed and blocks all future messages from a peer. This isn't obviously clear from reading the code, so document it.
Top commit has no ACKs.
Tree-SHA512: 4f8e43bbe6534242facfcfffae28b7a6aa2d228841fa2146a87d494e69f614b0da23cf7a5f3d4367358a7c1981fe2ec196a21c437ae1653f1c7e0351be22598a
099e4b9ad3 Set AA_EnableHighDpiScaling attribute early (Hennadii Stepanov)
Pull request description:
Running `bitcoin-qt` compiled against Qt 5.12.4 causes a warning:
```
hebasto@bionic-qt:~/bitcoin$ src/qt/bitcoin-qt
Attribute Qt::AA_EnableHighDpiScaling must be set before QCoreApplication is created.
```
This PR fixes this issue.
From Qt docs:
- [Qt::AA_EnableHighDpiScaling](https://doc.qt.io/qt-5/qt.html#ApplicationAttribute-enum):
> Enables high-DPI scaling in Qt on supported platforms (see also High DPI Displays). _Supported platforms are X11, Windows and Android._ Enabling makes Qt scale the main (device independent) coordinate system according to display scale factors provided by the operating system. This corresponds to setting the `QT_AUTO_SCREEN_SCALE_FACTOR` environment variable to 1. This attribute must be set before `QGuiApplication` is constructed. This value was added in Qt 5.6.
- [QCoreApplication::setAttribute()](https://doc.qt.io/qt-5/qcoreapplication.html#setAttribute)
ACKs for commit 099e4b:
MarcoFalke:
ACK 099e4b9ad3
jonasschnelli:
utACK 099e4b9ad3
fanquake:
ACK 099e4b9ad3. Did some testing on `Bionic` and `Windows 10` (using VirtualBox). I couldn't see any obvious visual difference, but given Marco's screens above, this change is obviously better. I also checked that there wasn't any sort of regression on macOS.
Tree-SHA512: 1965a427ee14ffb3871bac317685032406cf02d1fa2b2dc11c8b643bfe4ba09195674d149d1e41752f14c0d000446b35e142f3ce60d987ba97082fd7ee39a094
a2aabfb749 Use qInfo() if no error occurs (Hennadii Stepanov)
Pull request description:
[Warning and Debugging Messages](https://doc.qt.io/qt-5/debug.html#warning-and-debugging-messages):
> - `qInfo()` is used for informational messages.
> - `qWarning()` is used to report warnings and recoverable errors in your
application.
>
> If the `QT_FATAL_WARNINGS` environment variable is set, `qWarning()` exits after printing the warning message. This makes it easy to obtain a backtrace in the debugger.
[`qWarning()`](https://doc.qt.io/qt-5/qtglobal.html#qWarning):
> Calls the message handler with the warning message message... This function does nothing if `QT_NO_WARNING_OUTPUT` was defined during compilation; it exits if at the nth warning corresponding to the counter in environment variable `QT_FATAL_WARNINGS`.
This PR allows more productive debugging using the environment variable `QT_FATAL_WARNINGS`.
Examples:
- https://github.com/bitcoin/bitcoin/pull/16118#issuecomment-503184695
- https://github.com/bitcoin/bitcoin/pull/16254#issuecomment-504223404
The behavior, when option `-debug=qt` is set/unset, remains unchanged.
ACKs for commit a2aabf:
promag:
ACK a2aabfb, I also have this change locally.
Empact:
ACK a2aabfb749
laanwj:
ACK a2aabfb749
fanquake:
ACK a2aabfb749.
Tree-SHA512: b4df300c9c00a1705b0d3a10227e3deaac19a98b0a898bb60d5a88872cf450fb131eba150d9dd6c29e021566ee04b3b86b7d486bbe28bd894743c128d2309155
By checking the shutdown flag every loop we can use the entire nonce space
instead of breaking every 16 bits to check the shutdown flag.
This has been possible since the shutdown flag was switched to an atomic,
before that change it was controlled by a condition variable and lock.
5224be5a33 gui: Fix open wallet menu initialization order (João Barbosa)
Pull request description:
Fixes#16230, the menu must be created before connecting to aboutToShow signal.
ACKs for commit 5224be:
hebasto:
ACK 5224be5a33, I have tested the code on Bionic with Qt 5.12.4.
ryanofsky:
utACK 5224be5a33. Looks good, fix is simple and makes perfect sense after seeing explanation in https://github.com/bitcoin/bitcoin/pull/16118#issuecomment-503166407. Without this change (and since #16118), the menu pointer passed to `connect(m_open_wallet_action->menu(), ...)` is null and connecting has no effect. With this change, the menu is constructed earlier so the connect call can work.
fanquake:
ACK 5224be5a33 Testing included in a comment above. The segfaulting with QT_FATAL_WARNINGS is unrelated to this change.
Tree-SHA512: 97b42493b37b96683058bccf39a0ee93589293d4ba8f0c60aef7f4fb9dd084cc6d5608cd5ef531cadf5e03b1f01627ef96bc2d79f784fb38cb87aa6643183d41
71d0344cf2 docs: release note wording (Karl-Johan Alm)
3d2ff37913 wallet/rpc: use static help text (Karl-Johan Alm)
53c3c1ea9e wallet/rpc/getbalances: add entry for 'mine.used' balance in results (Karl-Johan Alm)
Pull request description:
This addresses a few remaining issues pointed out in #13756:
* First commit addresses https://github.com/bitcoin/bitcoin/pull/13756#discussion_r284907468
* Second commit addresses https://github.com/bitcoin/bitcoin/pull/13756#discussion_r294868973
Ping jnewbery and achow101 as they pointed out these issues.
ACKs for commit 71d034:
jnewbery:
ACK 71d0344cf2
meshcollider:
re-utACK 71d0344cf2
Tree-SHA512: 5e28822af0574ad07dbbed21aa2fe7866bf5770b4c0a1c150ad0da8af3152bcfb7170330a7497fa500326c594740ecf63733cf58325821e2811d7b911d5783a0
Always show the same help topic regardless of wallet flags, and explain that something is not always available, rather than runtime-modifying the help output.
e61de6306f Change ismine to take a CWallet instead of CKeyStore (Andrew Chow)
7c611e2000 Move ismine to wallet module (Andrew Chow)
Pull request description:
`IsMine` isn't used outside of the wallet except for the tests. It also doesn't make sense to be outside of the wallet. This PR moves `IsMine` into the wallet module and for it to take a `CWallet` instead of `CKeyStore`. The test that used `IsMine` is also moved to the wallet tests.
This is first [prerequisites](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Class-Structure-Changes#ismine) for the wallet structure changes.
ACKs for commit e61de6:
MarcoFalke:
re-ACK e61de6306f (only change is rebase with git auto-merge)
meshcollider:
Very light code review ACK e61de6306f
Tree-SHA512: 1cb4ad12652aef7922ab7460c6d413e8b9d1855dca78c0a286ae49d5c0765bc7996c55f262c742001d434eb9bd4215dc2cc7aae1b371ee1a82d46b32c17e6341
a49503402b Make and get the multisig redeemscript and destination in one function instead of two (Andrew Chow)
Pull request description:
`CreateMultisigRedeemscript()` is changed to `AddAndGetMultisigDestination()` so that the process of constructing the redeemScript and then getting the `CTxDestination` are done in the same function. This allows that function to see what the keys in the multisig are so that the correct address type is returned from `AddAndGetDestinationForScript()`.
This only effects the `createmultisig` and `addmultisigaddress` RPCs and does not change signing logic as #16022 does.
Alternative to #16022 and #16012Fixes#16011
ACKs for commit a49503:
Tree-SHA512: 5b0154a714deea3b2cc3a54beb420c95eeeacf4ca30c40ca80940d9d640f8b03611b0fc14c2f0710bfd8a79e8d27ad7d9ae380b4b83d52b40ab201624f2a63f0
Instead of creating a redeemScript with CreateMultisigRedeemscript and
then getting the destination with AddAndGetDestinationForScript, do
both in the same function.
CreateMultisigRedeemscript is changed to AddAndGetMultisigDestination.
It creates the redeemScript and returns it via an output parameter. Then
it calls AddAndGetDestinationForScript to add the destination to the
keystore and get the proper destination.
This allows us to inspect the public keys in the redeemScript before creating
the destination so that the correct destination is used when uncompressed
pubkeys are in the multisig.
5ebc6b0eb2 bitcoind: update -avoidpartialspends description to account for auto-enable for avoid_reuse wallets (Karl-Johan Alm)
ada258f8c8 doc: release notes for avoid_reuse (Karl-Johan Alm)
27669551da wallet: enable avoid_partial_spends by default if avoid_reuse is set (Karl-Johan Alm)
8f2e208f7c test: add test for avoidreuse feature (Karl-Johan Alm)
0bdfbd34cf wallet/rpc: add 'avoid_reuse' option to RPC commands (Karl-Johan Alm)
f904723e0d wallet/rpc: add setwalletflag RPC and MUTABLE_WALLET_FLAGS (Karl-Johan Alm)
8247a0da3a wallet: enable avoid_reuse feature (Karl-Johan Alm)
eec15662fa wallet: avoid reuse flags (Karl-Johan Alm)
58928098c2 wallet: make IsWalletFlagSet() const (Karl-Johan Alm)
129a5bafd9 wallet: rename g_known_wallet_flags constant to KNOWN_WALLET_FLAGS (Karl-Johan Alm)
Pull request description:
Add a new wallet flag called `avoid_reuse` which, when enabled, will keep track of when a specific destination has been spent from, and will actively "blacklist" any new UTXOs which send to an already-spent-from destination.
This improves privacy, as a payer could otherwise begin tracking a payee's wallet by regularly peppering a known UTXO with dust outputs, which would then be scooped up and used in payments by the payee, allowing the payer to map out (1) the inputs owned by the payee and (2) the destinations to which the payee is making payments.
This replaces #10386 and together with the (now merged) #12257 it addresses #10065 in full. The concerns raised in https://github.com/bitcoin/bitcoin/pull/10386#issuecomment-302361381 are also addressed due to #12257.
~~Note: this builds on top of #15780.~~ (merged)
ACKs for commit 5ebc6b:
jnewbery:
ACK 5ebc6b0eb
laanwj:
Concept and code-review ACK 5ebc6b0eb2
meshcollider:
Code review ACK 5ebc6b0eb2
achow101:
ACK 5ebc6b0eb2 modulo above nits
Tree-SHA512: fdef45826af544cbbb45634ac367852cc467ec87081d86d08b53ca849e588617e9a0a255b7e7bb28692d15332de58d6c3d274ac003355220e4213d7d9070742e
0959d37e3e Don't use global (external) symbols for symbols that are used in only one translation unit (practicalswift)
Pull request description:
Don't use global (external) symbols for symbols that are used in only one translation unit.
Before:
```
$ for SYMBOL in $(nm src/bitcoind | grep -E ' [BD] ' | c++filt | cut -f3- -d' ' | grep -v @ | grep -v : | sort | grep '[a-z]' | sort -u | grep -vE '(^_|typeinfo|vtable)'); do
REFERENCES=$(git grep -lE "([^a-zA-Z]|^)${SYMBOL}([^a-zA-Z]|\$)" -- "*.cpp" "*.h")
N_REFERENCES=$(wc -l <<< "${REFERENCES}")
if [[ ${N_REFERENCES} > 1 ]]; then
continue
fi
echo "Global symbol ${SYMBOL} is used in only one translation unit: ${REFERENCES}"
done
Global symbol g_chainstate is used in only one translation unit: src/validation.cpp
Global symbol g_ui_signals is used in only one translation unit: src/ui_interface.cpp
Global symbol instance_of_cmaincleanup is used in only one translation unit: src/validation.cpp
Global symbol instance_of_cnetcleanup is used in only one translation unit: src/net.cpp
Global symbol instance_of_cnetprocessingcleanup is used in only one translation unit: src/net_processing.cpp
Global symbol pindexBestForkBase is used in only one translation unit: src/validation.cpp
Global symbol pindexBestForkTip is used in only one translation unit: src/validation.cpp
$
```
After:
```
$ for SYMBOL in $(nm src/bitcoind | grep -E ' [BD] ' | c++filt | cut -f3- -d' ' | grep -v @ | grep -v : | sort | grep '[a-z]' | sort -u | grep -vE '(^_|typeinfo|vtable)'); do
REFERENCES=$(git grep -lE "([^a-zA-Z]|^)${SYMBOL}([^a-zA-Z]|\$)" -- "*.cpp" "*.h")
N_REFERENCES=$(wc -l <<< "${REFERENCES}")
if [[ ${N_REFERENCES} > 1 ]]; then
continue
fi
echo "Global symbol ${SYMBOL} is used in only one translation unit: ${REFERENCES}"
done
$
```
♻️ Think about future generations: save the global namespace from unnecessary pollution! ♻️
ACKs for commit 0959d3:
Empact:
ACK 0959d37e3e
MarcoFalke:
ACK 0959d37e3e
hebasto:
ACK 0959d37e3e
promag:
ACK 0959d37.
Tree-SHA512: 722f66bb50450f19b57e8a8fbe949f30cd651eb8564e5787cbb772a539bf3a288c048dc49e655fd73ece6a46f6dafade515ec4004729bf2b3ab83117b7c5d153
faa2a47cd7 logging: Add threadsafety comments (MarcoFalke)
0b282f9b00 Log early messages with -printtoconsole (Anthony Towns)
412987430c Replace OpenDebugLog() with StartLogging() (Anthony Towns)
Pull request description:
Early log messages are dropped on the floor and they'd never make it to the console or debug log. This can be tested by running the test included in this pull request without re-compiling the `bitcoind`.
Fix that by buffering early messages and flushing them as soon as all logging options have been initialized and logging has been started.
This pull request is identical to "Log early messages with -printtoconsole" (#13088) by **ajtowns**, with the following changes:
* Rebased
* Added docstrings for `m_buffering` and `StartLogging`
* Switch `CCriticalSection` (aka `RecursiveMutex`) to just `Mutex` in the last commit
* Added tests
Fixes#16098Fixes#13157Closes#13088
ACKs for commit faa2a4:
ajtowns:
utACK faa2a47cd7
hebasto:
ACK faa2a47cd7
kristapsk:
ACK faa2a47cd7 (ran added functional test before / after recompiling, didn't do additional testing)
Tree-SHA512: 685e2882642fe2a43ce171d42862582dadb840d03cda8236a994322c389ca2a1f3f431b179b2726c155c61793543bb340c568a5455d97f8b83bc7d307a85d387
8a2656702b torcontrol: Use the default/standard network port for Tor hidden services, even if the internal port is set differently (Luke Dashjr)
Pull request description:
Currently, the hidden service is published on the same port as the public listening port.
But if a non-standard port is configured, this can be used to guess (pretty reliably) that the public IP and the hidden service are the same node.
ACKs for top commit:
practicalswift:
utACK 8a2656702b
naumenkogs:
utACK 8a26567
laanwj:
utACK 8a2656702b
Tree-SHA512: 737c8da4f7c3f0bb22a338647d357987f5808156e3f38864168d0d8c2e2b171160812f7da4de11eef602902b304e357d76052950b72d7b3b83535b0fdd05fadc
86b47fa741 speed up Unserialize_impl for prevector (Akio Nakamura)
Pull request description:
The unserializer for prevector uses `resize()` for reserve the area, but it's prefer to use `reserve()` because `resize()` have overhead to call its constructor many times.
However, `reserve()` does not change the value of `_size` (a private member of prevector).
This PR make the logic of read from stream to callback function, and prevector handles initilizing new values with that call-back and ajust the value of `_size`.
The changes are as follows:
1. prevector.h
Add a public member function named 'append'.
This function has 2 params, number of elemenst to append and call-back function that initilizing new appended values.
2. serialize.h
In the following two function:
- `Unserialize_impl(Stream& is, prevector<N, T>& v, const unsigned char&)`
- `Unserialize_impl(Stream& is, prevector<N, T>& v, const V&)`
Make a callback function from each original logic of reading values from stream, and call prevector's `append()`.
3. test/prevector_tests.cpp
Add a test for `append()`.
## A benchmark result is following:
[Machine]
MacBook Pro (macOS 10.13.3/i7 2.2GHz/mem 16GB/SSD)
[result]
DeserializeAndCheckBlockTest => 22% faster
DeserializeBlockTest => 29% faster
[before PR]
# Benchmark, evals, iterations, total, min, max, median
DeserializeAndCheckBlockTest, 60, 160, 94.4901, 0.0094644, 0.0104715, 0.0098339
DeserializeBlockTest, 60, 130, 65.0964, 0.00800362, 0.00895134, 0.00824187
[After PR]
# Benchmark, evals, iterations, total, min, max, median
DeserializeAndCheckBlockTest, 60, 160, 77.1597, 0.00767013, 0.00858959, 0.00805757
DeserializeBlockTest, 60, 130, 49.9443, 0.00613926, 0.00691187, 0.00635527
ACKs for top commit:
laanwj:
utACK 86b47fa741
Tree-SHA512: 62ea121ccd45a306fefc67485a1b03a853435af762607dae2426a87b15a3033d802c8556e1923727ddd1023a1837d0e5f6720c2c77b38196907e750e15fbb902
8053e5cdad Remove -mempoolreplacement to prevent needless block prop slowness. (Matt Corallo)
Pull request description:
At this point there is no reasonable excuse to disable opt-in RBF,
and, unlike when this option was added, there are now significant
issues created when disabling it (in the form of compact block
reconstruction failures). Further, it breaks a lot of modern wallet
behavior.
This removes an option that is:
* (a) only useful when a large portion of (other) miners enforce it as well
* (b) is detrimental to everyone (income for miners, RBF notifications for others) who uses it individually otherwise
* (c) is effectively unused
* (d) is often confused with disabling RBF (rather than just remaining stubbornly unaware of it while the rest of the network lets it through)
ACKs for commit 8053e5:
practicalswift:
utACK 8053e5cdad
promag:
Deprecation would save from unlikely rantings, still ACK 8053e5c.
jtimon:
utACK 8053e5cdad
ajtowns:
ACK 8053e5cdad -- quick code review, checked tests work
MarcoFalke:
ACK 8053e5cdad
Tree-SHA512: 01aee8905b2487fc38a3a86649d422d2d2345bc60f878889ebda4b8680783e1f1a97c2000c27ef086719501be2abc2911b2039a259a5e5c04f3b24ff02b0427e
fa499b5f02 rpc: bugfix: Properly use iswitness in converttopsbt (MarcoFalke)
fa5c5cd141 rpc: Switch touched RPCs to IsValidNumArgs (MarcoFalke)
Pull request description:
When a serialized transaction has inputs, there is no risk in only trying to deserialize it with witness allowed. (This is how all transactions from p2p are deserialized.) In fact, it would avoid a common issue where a transaction with inputs can be deserialized in two ways:
* Fixes#12989
* Fixes#15872
* Fixes#15701
* Fixes#13738
* ...
When a serialized transaction has no inputs, there is no risk in only trying to deserialze it with witness disallowed. (A transaction without inputs can't have corresponding witness data)
ACKs for commit fa499b:
meshcollider:
utACK fa499b5f02
ryanofsky:
utACK fa499b5f02. Changes since last review: consolidating commits and making iswitness documentation the same across methods.
PastaPastaPasta:
utACK fa499b5f02
Tree-SHA512: a64423a3131f3f0222a40da557c8b590c9ff01b45bcd40796f77a1a64ae74c6680a6be9d01ece95c492dfbcc7e2810409d2c2b336c2894af00bb213972fc85c6
c59e3a3261 getrawtransaction: inform about blockhash argument when lookup fails (darosior)
Pull request description:
Just 4 words added on `getrawtransaction` lookup error to fix#16142
ACKs for commit c59e3a:
Tree-SHA512: 2219099c1240667527a9b1498a58818b5ff1c2ef366c498d2bb57963e828b3c87fa3e6b94be7e6463bd289ceabc13f9c9b1082134641594ba335ac400e6d63aa
fa8f195195 Replace remaining fprintf with tfm::format manually (MarcoFalke)
fac03ec43a scripted-diff: Replace fprintf with tfm::format (MarcoFalke)
fa72a64b90 tinyformat: Add doc to Bitcoin Core specific strprintf (MarcoFalke)
Pull request description:
This should be a refactor except in the cases where we use the wrong format specifier [1], in which case this patch is a bug fix.
[1] : e.g. depends: Add libevent compatibility patch for windows #8730
ACKs for commit fa8f19:
promag:
ACK fa8f195195. Ideally this should be rebased before merge.
practicalswift:
utACK fa8f195195
Empact:
ACK fa8f195195
laanwj:
code review and lightly tested ACK fa8f195195
jonatack:
ACK fa8f195195 from light code review, building, and running linter/unit tests/extended functional tests.
Tree-SHA512: 65f648b0bc383e3266a5bdb4ad8c8a1908a719635d49e1cd321b91254be24dbc7e22290370178e29b98ddcb3fec0889de9cbae273c7140abc9793d849534a743
b748bf6f50 Fix spelling errors identified by codespell 1.15.0 (Ben Woosley)
Pull request description:
Note all changes are to comments / documentation.
After this commit, the only remaining output is:
```
$ test/lint/lint-spelling.sh
src/test/base32_tests.cpp:14: fo ==> of, for
src/test/base64_tests.cpp:14: fo ==> of, for
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/lint-spelling.ignore-words.txt
```
Note:
* I ignore several valid alternative spellings ~, but changed homogenous
to homogeneous as the latter is a more specific term according to the
Google dictionary definitions I found~
* homogenous is present in tinyformat, hence should be addressed upstream
* process' is correct only if there are plural processes
ACKs for commit b748bf:
practicalswift:
ACK b748bf6f50
fanquake:
ACK b748bf6f50
Tree-SHA512: 9add7044643ce015e0a44d8b27a3f300d72c485ffff550fb6491a17f14528085289ec5caddfe02f291ea9b2cded38a0dd3079652a054e2d7fe2ff4f7b53db5d7
f402012cc fixup: Fix prunning test (João Barbosa)
97f517dd8 Fix RPC/pruneblockchain returned prune height (Jonas Schnelli)
Pull request description:
The help of `pruneblockchain` tells us that the return value is `Height of the last block pruned.`,... but the implementation naively returns the provided input `height` and therefore not respecting that pruning can't be done on all possible blockheight due to the fact that we only prune complete blockfiles (which combine multiple blocks).
This fixes the return value to actually return the correct prune height.
ACKs for commit f40201:
MarcoFalke:
ACK f402012ccf
Tree-SHA512: 88c910030ffb83196663e5ebebc29d036fcdbbb2ab266e4538991867924a61bacd8361c1fbf294a0ea7e02347ae183d792f10a10b8f6187e8a4c4c6e4124d7e6
75485ef09 gui: Enable open wallet menu on setWalletController (João Barbosa)
Pull request description:
`BitcoinApplication::initializeResult` and `BitcoinGUI::setWalletController` are only called after the startup rescan is completed. While the rescan is in progress the window menus are already available.
This PR makes the Open Wallet menu disabled until `BitcoinGUI::setWalletController` is called.
Fixes#16087
ACKs for commit 75485e:
jonasschnelli:
utACK 75485ef096
ryanofsky:
utACK 75485ef096. It's a simple, sensible fix.
Tree-SHA512: 9395ceed54bbceb6cbf1cd443f783d07a6ebb8fc5515b63c6e1b8b19b216b08d1cba7eaf872814d7c426ab7192f3b416ba0d57fc84f3bcbfebf01ce153794201
308b76732f Fix bug around transaction requests (Suhas Daftuar)
f635a3ba11 Expire old entries from the in-flight tx map (Suhas Daftuar)
e32e08407e Remove NOTFOUND transactions from in-flight data structures (Suhas Daftuar)
23163b7593 Add an explicit memory bound to m_tx_process_time (Suhas Daftuar)
218697b645 Improve NOTFOUND comment (Suhas Daftuar)
Pull request description:
#14897 introduced several bugs that could lead to a node no longer requesting transactions from one or more of its peers. Credit to ajtowns for originally reporting many of these bugs along with an originally proposed fix in #15776.
This PR does a few things:
- Fix a bug in NOTFOUND processing, where the in-flight map for a peer was keeping transactions it shouldn't
- Eliminate the possibility of a memory attack on the CNodeState `m_tx_process_time` data structure by explicitly bounding its size
- Remove entries from a peer's in-flight map after 10 minutes, so that we should always eventually resume transaction requests even if there are other bugs like the NOTFOUND one
- Fix a bug relating to the coordination of request times when multiple peers announce the same transaction
The expiry mechanism added here is something we'll likely want to remove in the future, but is belt-and-suspenders for now to try to ensure we don't have other bugs that could lead to transaction relay failing due to some unforeseen conditions.
ACKs for commit 308b76:
ajtowns:
utACK 308b76732f
morcos:
light ACK 308b767
laanwj:
Code review ACK 308b76732f
jonatack:
Light ACK 308b76732f.
jamesob:
ACK 308b76732f
MarcoFalke:
ACK 308b76732f (Tested two of the three bugs this pull fixes, see comment above)
jamesob:
Concept ACK 308b76732f
MarcoFalke:
ACK 308b76732f
Tree-SHA512: 8865dca5294447859d95655e8699085643db60c22f0719e76e961651a1398251bc932494b68932e33f68d4f6084579ab3bed7d0e7dd4ac6c362590eaf9414eda
After this commit, the only remaining output is:
$ test/lint/lint-spelling.sh
src/test/base32_tests.cpp:14: fo ==> of, for
src/test/base64_tests.cpp:14: fo ==> of, for
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/lint-spelling.ignore-words.txt
Note:
* I ignore several valid alternative spellings
* homogenous is present in tinyformat, hence should be addressed upstream
* process' is correct only if there are plural processes
At this point there is no reasonable excuse to disable opt-in RBF,
and, unlike when this option was added, there are now significant
issues created when disabling it (in the form of compact block
reconstruction failures). Further, it breaks a lot of modern wallet
behavior.
53b7de629d Add test for dumping the private key imported from descriptor (MeshCollider)
2857bc4a64 Extend importmulti descriptor tests (MeshCollider)
81a884bbd0 Import private keys from descriptor with importmulti if provided (MeshCollider)
a4d1bd1a29 Add private key derivation functions to descriptors (MeshCollider)
Pull request description:
~This is based on #14491, review the last 3 commits only.~
Currently, descriptors have an Expand() function which returns public keys and scripts for a specific index of a ranged descriptor. But the private key for a specific index is not given. This allows private keys for specific indices to be derived. This also allows those keys to be imported through the `importmulti` RPC rather than having to provide them separately.
ACKs for commit 53b7de:
achow101:
ACK 53b7de629d
Tree-SHA512: c060bc01358a1adc76d3d470fefc2bdd39c837027f452e9bc4bd2e726097e1ece4af9d5627efd942a5f8819271e15ba54f010b169b50a9435a1f0f40fd1cebf3
67f4e9c522 Include core_io.h from core_read.cpp (practicalswift)
eca9767673 Make reasoning about dependencies easier by not including unused dependencies (practicalswift)
Pull request description:
Make reasoning about dependencies easier by not including unused dependencies.
Please note that the removed headers are _not_ "transitively included" by other still included headers. Thus the removals are real.
As an added bonus this change means less work for the preprocessor/compiler. At least 51 393 lines of code no longer needs to be processed:
```
$ git diff -u HEAD~1 | grep -E '^\-#include ' | cut -f2 -d"<" | cut -f1 -d">" | \
sed 's%^%src/%g' | xargs cat | wc -l
51393
```
Note that 51 393 is the lower bound: the real number is likely much higher when taking into account transitively included headers :-)
ACKs for commit 67f4e9:
Tree-SHA512: 0c8868aac59813f099ce53d5307eed7962dd6f2ff3546768ef9e5c4508b87f8210f1a22c7e826c3c06bebbf28bdbfcf1628ed354c2d0fdb9a31a42cefb8fdf13
BIP 37 bloom filters have been well-known to be a significant DoS
target for some time. However, in order to provide continuity for
SPV clients relying on it, the NODE_BLOOM service flag was added,
and left as a default, to ensure sufficient nodes exist with such a
flag.
NODE_BLOOM is, at this point, well-established and, as long as
there exist 0.18 nodes with default config (which I'd anticipate
will be true for many years), will be available from some peers. By
that time, the continued slowdown of BIP 37-based filtering will
likely have rendered it useless (though this is already largely the
case). Further, BIP 37 was deliberately never updated to support
witness-based filtering as newer wallets are expected to migrate to
some yet-to-be-network-exposed filters.
8be3f3063 netaddress: Update CNetAddr for ORCHIDv2 (Carl Dong)
Pull request description:
```
The original ORCHID prefix was deprecated as of 2014-03, the new
ORCHIDv2 prefix was allocated by RFC7343 as of 2014-07. We did not
consider the original ORCHID prefix routable, and I don't see any reason
to consider the new one to be either.
```
Would like to know if people think this kind of thing is even worth keeping the codebase updated for. Perhaps it'd be nice to write a devtool to pull the csv from [here](https://www.iana.org/assignments/iana-ipv6-special-registry/iana-ipv6-special-registry.xhtml) and generate the code.
ACKs for commit 8be3f3:
laanwj:
utACK 8be3f3063
ryanofsky:
utACK 8be3f30633. Only change since last review is rebasing after #15718 merge.
Tree-SHA512: 7c93317f597b1a6c1443e12dd690010392edb9d72a479a8201970db7d3444fbb99a80b98026caad6fbfbebb455ab4035d2dde79bc9263bfd1d0398cd218392e1
Commit fbf327b138 ("Minimal code
changes to allow msvc compilation.") was indeed minimal in terms
of lines touched. But as a result of that minimalism it changed the
logic in memory_cleanse() to first call std::memset() and then
additionally the MSVC-specific SecureZeroMemory() function, and it
also moved a comment to the wrong location.
This commit removes the superfluous call to std::memset() on MSVC
and ensures that the comment is in the right position again.
58e291cfa Add test for GCC bug 90348 (Pieter Wuille)
Pull request description:
This adds a test for GCC bug 90348 (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90348), using a test case extracted from our own `sha256d64` test in crypto_tests.cpp, which was failing on some platforms.
This is based on top of #15983 to make sure the bug doesn't trigger (it does in some Travis configurations without it).
ACKs for commit 58e291:
Tree-SHA512: 4dc9084e92dd143a53930e42bb68e33d922a2a2b891406b259d3a0bed4511dcc49e7447a7a8e4eb793a26e3eacb188ca293b71e0e061f9b3230f8e7fcfd29525
7860c98bd wallet: do not encrypt wallets with disabled private keys (whythat)
Pull request description:
Fix for #15635.
Throw an `RPC_WALLET_ENCRYPTION_FAILED` error when attempting to encrypt wallet with disabled private keys. Changed `test/function/wallet_createwallet.py` to test new behavior.
ACKs for commit 7860c9:
achow101:
utACK 7860c98bd5
meshcollider:
utACK 7860c98bd5
Tree-SHA512: d0cc40efd303a00d0b4d3cb2de59d8d2d7dd35647e7f3fe9d4a8986589499c1f567c5780c83a129e1ab8dbe601279c459c6ebce3b48b1d81d47a28616ef4a369
403e677c9 refactoring: IsInitialBlockDownload -> CChainState (James O'Beirne)
3ccbc376d refactoring: FlushStateToDisk -> CChainState (James O'Beirne)
4d6688603 refactoring: introduce ChainstateActive() (James O'Beirne)
d7c97edee move-only: make the CChainState interface public (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11):
Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/2019-04-proposal/proposal
---
This changeset starts moving functionality intimately related to CChainState into methods. Parameterizing these functions by a particular CChainState is necessary for the use of multiple chainstates simultaneously (e.g. for asynchronous background validation).
In this change, we
- make the CChainState interface public - since other units will start to invoke its methods directly,
- introduce `::ChainstateActive()`, the CChainState equivalent for `::ChainActive()`,
- and move `IsInitialBlockDownload()` and `FlushStateToDisk()` into methods on CChainState.
Independent of assumeutxo, these changes better encapsulate chainstate behavior and allow easier use from a testing context.
There are more methods that we'll move in the future, but they require other substantial changes (i.e. moving ownership of the `CCoinsView*` hierarchy into CChainState) so we'll save them for future PRs.
---
The first move-only commit is most easily reviewed with `git diff ... --color-moved=dimmed_zebra`.
ACKs for commit 403e67:
Empact:
utACK 403e677c9e no need to address my nits herein
Sjors:
utACK 403e677
ryanofsky:
utACK 403e677c9e. Only change since previous review is removing global state comment as suggested.
MarcoFalke:
utACK 403e677c9e, though the diff still seems a bit bloated with some unnecessary changes in the second commit.
promag:
utACK 403e677 and rebased with current [master](c7cfd20a7).
Tree-SHA512: 6fcf260bb2dc201361170c0b4547405366f5f331fcc3a2bac29b24442814b7b244ca1b58aac5af716885f9a130c343b544590dff780da0bf835c7c5b3ccb2257
If we're unable to disconnect a block during normal operation, then that is a
failure of our local system (such as disk failure) or the chain that we are on
(eg CVE-2018-17144), but cannot be due to failure of the (more work) chain that
we're trying to validate.
We should abort rather than stay on a less work chain.
6e6494b3fb qt: fix opening bitcoin.conf via Preferences on macOS; see #15409 (shannon1916)
Pull request description:
Fix#15409. The QT wallet fail to open the configuration file on Mac, when these is no default application for `*.conf` files.
Here is a feasible way to solve this bug. When `QDesktopServices::openUrl` fails to open `file:///path/bitcoin.conf` with its default application, use `QProcess::startDetached` to run `open -t /path/bitcoin.conf` command instead, so as to open the configuration file with system's default text editor.
ACKs for commit 6e6494:
hebasto:
re-ACK 6e6494b3fb
fanquake:
tACK 6e6494b3fb on macOS 10.14.x
Tree-SHA512: 60e898f4cb77cfd7b8adbc8d33fbebf46bac2a801bdcf40cae15e24b78ad56b1f32358b1879b670623d9f8651dea93961d34269358cea18f4e15b089a8ffcfbf
2d8ad2f997 gui: Enable console line edit on setClientModel (João Barbosa)
Pull request description:
Make console line edit disable by default, and only enable once `RPCConsole::setClientModel` is called.
Fixes#16119.
ACKs for commit 2d8ad2:
fanquake:
tACK 2d8ad2f997 on macOS.
Tree-SHA512: 1418ce3c120c08e5ec3e7a7a063572a24402ce0ec541bd4adc21f61d60c4e86b711e82e940ebf5f0445ab861f89c146c2a2e7990fb52bed2c65fc199a1981f71
54245985fb Squashed 'src/secp256k1/' changes from 0b70241850..b19c000063 (Pieter Wuille)
Pull request description:
It's been 1.5 years since our secp256k1 subtree was updated, while the upstream project has undergone a number of incremental improvements (performance, tests, build system fixes), plus gained the groundwork for batch verification.
As we're early in the 0.19 window, this seems like a good time to get these merged.
ACKs for commit 99df27:
fanquake:
utACK 99df276 the subtree merge, still need to test the actual changes.
laanwj:
utACK 99df276da
Tree-SHA512: 769a699366321635068ebfbd9d3f30f6e72401c4fcdc1fdc84e5b3fd888c3f01437748f6cd23a507ab47cf04c226cd504fd48aee654457c34bb106c9db7e5c09
fa013664ae util: Add type safe GetTime (MarcoFalke)
Pull request description:
There are basically two ways to get the time in Bitcoin Core:
* get the system time (via `GetSystemTimeInSeconds` or `GetTime{Millis,Micros}`)
* get the mockable time (via `GetTime`)
Both return the same type (a plain int). This can lead to (test-only) bugs such as 99464bc38e.
Fix that by deprecating `GetTime` and adding a `GetTime<>` that returns the mockable time in a non-int type. The new util function is currently unused, but new code should it where possible.
ACKs for commit fa0136:
promag:
utACK fa013664.
Tree-SHA512: efab9c463f079fd8fd3030c479637c7b1e8be567a881234bd0f555c8f87e518e3b43ef2466128103db8fc40295aaf24e87ad76d91f338c631246fc703477e95c
fadbc5d895 mempool: remove unused magic number from consistency check (Gregory Sanders)
Pull request description:
Unexplained magic numbers are no good. Since the exact number does not matter, opt for a constant that is less peculiar.
Note that this could only possibly affect mempool consistency checks which is not active by default except on regtest.
see discussion: https://github.com/bitcoin/bitcoin/issues/15080
ACKs for commit fadbc5:
practicalswift:
utACK fadbc5d895
Tree-SHA512: 80f95ebc284c5bcc5d825fab0e9f962457a411539946d68ef4c8bdea4b1f2f7f0ead88928fac0eaaa02a1175f01f5ef381613ce53b0f27c3098e90d76ecfe9af
0db94e55d wallet: Pass WalletBatch to CWallet::UnsetWalletFlag (João Barbosa)
6cb888b37 Apply the batch treatment to CWallet::SetAddressBook via ImportScriptPubKeys (Ben Woosley)
6154a09e0 Move some of ProcessImport into CWallet::Import* (Ben Woosley)
ccb26cf34 Batch writes for importmulti (Andrew Chow)
d6576e349 Have WalletBatch automatically flush every 1000 updates (Andrew Chow)
366fe0be0 Add AddWatchOnlyWithDB, AddKeyOriginWithDB, AddCScriptWithDB functions (Andrew Chow)
Pull request description:
Instead of writing each item to the wallet database individually, do them in batches so that the import runs faster.
This was tested by importing a ranged descriptor for 10,000 keys.
Current master
```
$ time src/bitcoin-cli -regtest -rpcwallet=importbig importmulti '[{"desc": "sh(wpkh([73111820/44h/1h/0h]tpubDDoT2SgEjaU5rerQpfcRDWPAcwyZ5g7xxHgVAfPwidgPDKVjm89d6jJ8AQotp35Np3m6VaysfUY1C2g68wFqUmraGbzhSsMF9YBuTGxpBaW/1/*))#3w7php47", "range": [0, 10000], "timestamp": "now", "internal": true, "keypool": false, "watchonly": true}]'
...
real 7m45.29s
```
This PR:
```
$ time src/bitcoin-cli -regtest -rpcwallet=importbig4 importmulti '[{"desc": "pkh([73111820/44h/1h/0h]tpubDDoT2SgEjaU5rerQpfcRDWPAcwyZ5g7xxHgVAfPwidgPDKVjm89d6jJ8AQotp35Np3m6VaysfUY1C2g68wFqUmraGbzhSsMF9YBuTGxpBaW/1/*)#v65yjgmc", "range": [0, 10000], "timestamp": "now", "internal": true, "keypool": false, "watchonly": true}]'
...
real 3.93s
```
Fixes#15739
ACKs for commit 0db94e:
jb55:
utACK 0db94e5
ariard:
Tested ACK 0db94e5
Empact:
re-utACK 0db94e55dc only change is re the privacy of `UnsetWalletFlagWithDB` and `AddCScriptWithDB`.
Tree-SHA512: 3481308a64c99b6129f7bd328113dc291fe58743464628931feaebdef0e6ec770ddd5c19e4f9fbc1249a200acb04aaf62a8d914d53b0a29ac1e557576659c0cc
If a transaction is already in-flight when a peer announces a new tx to us, we
schedule a time in the future to reconsider whether to download. At that future
time, there was a bug that would prevent transactions from being rescheduled
for potential download again (ie if the transaction was still in-flight at the
time of reconsideration, such as from some other peer). Fix this.
If a peer hasn't responded to a getdata request, eventually time out the request
and remove it from the in-flight data structures. This is to prevent any bugs in
our handling of those in-flight data structures from filling up the in-flight
map and preventing us from requesting more transactions (such as the NOTFOUND
bug, fixed in a previous commit).
Co-authored-by: Anthony Towns <aj@erisian.com.au>
9f85e9cb3d scripted-diff: Rename LockAnnotation to LockAssertion (practicalswift)
de9b5dbca3 Make sure the compile-time locking promises given via LockAnnotation:s hold also in practice at runtime (ifdef DEBUG_LOCKORDER) (practicalswift)
3a809446b3 Move LockAnnotation to make it reflect the truth (practicalswift)
cc2588579c Move LockAnnotation from threadsafety.h (imported code) to sync.h (our code) (practicalswift)
Pull request description:
`LockAnnotation lock(mutex);` is a guarantee to the compiler thread-analysis that `mutex` is locked (when it couldn't be determined otherwise).
Before this PR it was possible to make the mistake of adding a `LockAnnotation` where the correct mutex is _not_ held. This in turn makes the thread-analysis reasoning being based on incorrect premises.
This PR adds an assertion in the `LockAnnotation` ctor which checks that the guarantees given by us at compile-time are held also in practice (`ifdef DEBUG_LOCKORDER`).
Issues like the one described in #16028 will be discovered immediately with this PR merged.
Changes in this PR:
* Move `LockAnnotation` from `threadsafety.h` (imported code) to `sync.h` (our code)
* Move `LockAnnotation` in `wallet_tests` to make it reflect the truth
* Make sure the compile-time locking promises given via `LockAnnotation`:s hold also in practice at runtime (`ifdef DEBUG_LOCKORDER`)
* Rename `LockAnnotation` to `LockAssertion`
ACKs for commit 9f85e9:
ryanofsky:
utACK 9f85e9cb3d. No changes at all since last review except clean rebase after base PR #16033 was merged
Tree-SHA512: fb80e78fe362adfd6ea8405bcb142c09b99f834fe8be4397282b223ca2c3a2bb9719a074a47a043b44757f840b239a6fcd2f98d14771f8729204834ecf608c3a
e23809a05b [rpc] deriveaddresses: Correct descriptor checksum in RPCExamples (Chris Capobianco)
Pull request description:
Trvial: This fixes the descriptor checksum found the in the deriveaddresses RPC example.
The current checksum value does work, but only if the "h" used for the hardened derivation key origin are replaced with "'".
Given the discussion to switch from "'" to "h" [here](https://github.com/bitcoin/bitcoin/issues/15740), I thought it made more sense to update the checksum rather then changing all the "h" to "'" in this example.
ACKs for commit e23809:
instagibbs:
tACK e23809a05b
Tree-SHA512: 06a2b9f3e714ecde9b9a80b3b7a4082eb072e71d8abcc455ff5387e470d48839f22a70b78bbae1cf9122cb133fee46830819b6f39d67aec8c3c8d5889ae94e04
1609809fb2 validation: Hold cs_main when reading chainActive in RewindBlockIndex (practicalswift)
Pull request description:
Fixes#15980.
Hold `cs_main` when reading `chainActive` (via `::ChainActive()`) in `RewindBlockIndex`.
ACKs for commit 160980:
MarcoFalke:
utACK 1609809fb2
Tree-SHA512: 54f180ab391f92f04950735c2bb337f0b7495826d2096f7a0f9a2da50bc29d08747f404a0495e33ca4edd4c842efbab4c4730d5e1a8b9da3e1249cf884268f4b
2c448d6bc7 parameterize hard coded numbers referring to miner conf window (Jordan Baczuk)
Pull request description:
Replace hard coded values (eg. 2016) with `mainnetParams.nMinerConfirmationWindow` where appropriate. This parameterizes hard coded values in the unit test that refer to the `Miner Confirmation Window`, which currently is `2016`. This includes values not exactly 2016 but which were derived from it. Also changed `int` to `uint32_t` where appropriate to avoid compiler warnings. This makes one source of truth, and also helps people who might be adjusting this value in testing so the unit tests don't break.
ACKs for commit 2c448d:
Tree-SHA512: 9262e0b89c1baf7857b49fe2221b2b00f948f61317b321c4871a9182a86d6f8aadeb59d6b133e8a213cc9b31b4a417888fb1ad31caef16ccbbab1de33c4b8459
fa2b52af32 Disallow extended encoding for non-witness transactions (take 3) (MarcoFalke)
Pull request description:
(previous title "p2p: Disallow extended encoding for non-witness transactions (take 3)")
Remote peers can send us illegally encoded txs and thus have us write to stderr. Fix that by not writing to stderr.
This is a follow up to the previous (incomplete) attempts at this:
* Disallow extended encoding for non-witness transactions #14039
* Add test for superfluous witness record in deserialization #15893
ACKs for commit fa2b52:
laanwj:
utACK fa2b52af32
ryanofsky:
utACK fa2b52af32. Would change title to something like "Avoid logging transaction decode errors to stderr" instead of "Disallow extended encoding for non-witness transactions." The current title is confusing because this PR isn't really allowing or disallowing anything, just logging the condition differently. "Disallow" also seems to contradict the "Allow exceptions from..." comments in the actual code.
Tree-SHA512: c66990e69b432d00dc1c5510bf976a1188664d0890a32d1e5c6459094e7e27da82a5d227627afcbc203676f5540eec74b7d9b1d71d2c62d3b2069e1781824b4d
0b09a57ae Give WalletModel::UnlockContext move semantics (Pieter Wuille)
Pull request description:
WalletModel::UnlockContext seems to implement "move upon copy" semantics; with C++11 this can be done more safely using move semantics (making attempts to actually copy fail instead).
Not a big deal if this isn't worth review time.
ACKs for commit 0b09a5:
Empact:
utACK 0b09a57aec
jonasschnelli:
utACK 0b09a57aec
jb55:
utACK 0b09a57aec
Tree-SHA512: f827856586afd03666c2d9f50320776afb3dd511ac1bcd293b330f015acd1588551b163dccc97b1351301e3295f4c74d90e5754bcee89faeadf6437d7db165c8
When writing all of the imported data to the wallet, use a common
WalletBatch object so that batch writes are done and the writes
finish more quickly.
AddKeypoolPubkey is no longer needed so it is also removed
AddWatchOnlyWithDB, AddKeyOriginWithDB, and AddCScriptWithDB add their
respective data to the wallet using the provided WalletBatch instead
of creating a new WalletBatch object every time. This allows for batching
writes to the database.
3cb9ce85d0 Document strenghtening (Pieter Wuille)
1d207bc46f Add hash strengthening to the RNG (Pieter Wuille)
Pull request description:
This patch improves the built-in RNG using hash strengthening.
At startup, and once every minute, 32 bytes of entropy are produced from the RNG, repeatedly hashed using SHA512 for 10ms, and then fed back into the RNG, together with high-precision timestamps obtained every 1000 iterations.
ACKs for commit 3cb9ce:
pstratem:
utACK 3cb9ce85d0
Tree-SHA512: 4fb6f61639b392697beb81c5f0903f79f10dd1087bed7f34de2abb5c22704a671e37b2d828ed141492491863efb1e7d1fa04408a1d32c9de2f2cc8ac406bbe57
fa86c8aec6 init: Remove dead code in LoadChainTip (MarcoFalke)
Pull request description:
`LoadChainTip` sets `::ChainActive()` based on `pcoinsTip`'s best block. `LoadChainTip` is never called when that block is null, so we can remove all code from within that method that is only executed when that block is null.
Fixes#15967 Inconsistent locking behavior in LoadChainTip
ACKs for commit fa86c8:
promag:
utACK fa86c8aec6.
practicalswift:
utACK fa86c8aec6
Empact:
utACK fa86c8aec6
laanwj:
utACK fa86c8aec6
ryanofsky:
utACK fa86c8aec6. LoadChainTip isn't called currently when pcoinsTip best block is null due to this line:
jamesob:
utACK fa86c8aec6
Tree-SHA512: 8961c0e579800a52038ac5655478468852faac055299b64d6cfdf0c213d3bf09669c4889467d09d93457f6c8b073967bb0475a137f77ddd3a3a3c03ad90001c4
8794a4b3ae QA: add test for HKDF HMAC_SHA256 L32 (Jonas Schnelli)
551d489416 Add HKDF HMAC_SHA256 L=32 implementations (Jonas Schnelli)
3b64f852e4 QA: add test for CKey::Negate() (Jonas Schnelli)
463921bb64 CKey: add method to negate the key (Jonas Schnelli)
Pull request description:
This adds a limited implementation of `HKDF` (defined by rfc5869) that supports only HMAC-SHA256 and length output of 32 bytes (will be required for v2 transport protocol).
This PR also includes a method to negate a private key which is useful to enforce public keys starting with 0x02 (or 0x03) (a requirement for the v2 transport protocol). The new `CKey::Negate()` method is pretty much a wrapper around `secp256k1_ec_privkey_negate()`.
Including tests.
This is a subset of #14032 and a pre-requirement for the v2 transport protocol.
ACKs for commit 8794a4:
Tree-SHA512: 5341929dfa29f5da766ec3612784baec6a3ad69972f08b5a985a8aafdae4dae36f104a2b888d1f5d1f33561456bd111f960d7e32c2cc4fd18e48358468f26c1a
fa8ced32a6 doc: Mention blocksonly in reduce-traffic.md, unhide option (MarcoFalke)
fa320de79f test: Add test for p2p_blocksonly (MarcoFalke)
fa3872e7b4 test: Format predicate source as multiline on error (MarcoFalke)
fa1dce7329 net: Rename ::fRelayTxes to ::g_relay_txes (MarcoFalke)
Pull request description:
This is de-facto no longer hidden
ACKs for commit fa8ced:
jamesob:
utACK fa8ced32a6
Tree-SHA512: 474fbdee6cbd035ed9068a066b6056c1f909ec7520be0417820fcd1672ab3069b53f55c5147968978d9258fd3a3933fe1a9ef8e4f6e14fb6ebbd79701a0a1245
662d1171d9 Add option to create an encrypted wallet (Andrew Chow)
Pull request description:
This PR adds a new `passphrase` argument to `createwallet` which will create a wallet that is encrypted with that passphrase.
This is built on #15226 because it needs to first create an empty wallet, then encrypt the empty wallet and generate new keys that have only been stored in an encrypted state.
ACKs for commit 662d11:
laanwj:
utACK 662d1171d9
jnewbery:
Looks great. utACK 662d1171d9
Tree-SHA512: a53fc9a0f341eaec1614eb69abcf2d48eb4394bc89041ab69bfc05a63436ed37c65ad586c07fd37dc258ac7c7d5e4f7f93b4191407f5824bbf063b4c50894c4a
fa7e311e16 [doc] rpcwallet: Only fail rescan when blocks have been pruned (MarcoFalke)
aaaa57c2aa scripted-diff: Bump copyright headers in wallet (MarcoFalke)
faf3729242 wallet: Only fail rescan when blocks have actually been pruned (MarcoFalke)
Pull request description:
This brings the behaviour of the import* calls closer to importmulti. After this change, the difference between importmulti and the other import* calls is
* that in importmulti you can "opt-out" of scanning early blocks by setting a later timestamp.
* that in importmulti the wallet will successfully import the data, but fail to rescan. Whereas in the other calls, the wallet will abort before importing the data.
ACKs for commit fa7e31:
promag:
utACK fa7e311e16.
jnewbery:
utACK fa7e311e16
Tree-SHA512: a57d52ffea94b64e0eb9b5d3a7a63031325833908297dd14eb0c5251ffea3b2113b131003f1db4e9599e014369165a57f107a7150bb65e4c791e5fe742f33cb8
c01c065b9d Do not construct out-of-bound pointers in SHA512/SHA1/RIPEMD160 code (Pieter Wuille)
Pull request description:
This looks like an issue in the current SHA256/512 code, where a pointer outside of the area pointed to may be constructed (this is UB in theory, though in practice every supported platform treats pointers as integers).
I discovered this while investigating #14580. Sadly, it does not fix it.
ACKs for commit c01c06:
practicalswift:
utACK c01c065b9d
Tree-SHA512: 47660e00f164f38c36a1ab46e52dd91cd33cfda6a6048d67541c2f8e73c050d4d9d81b5c149bfad281212d52f204f57bebf5b19879dc7a6a5f48aa823fbc2c02
Also renames global methods for clarity:
- ::FlushStateToDisk() -> CChainState::ForceFlushStateToDisk()
- This performs an unconditional flush.
- ::PruneAndFlush() -> CChainState::PruneAndFlush()
along with DisconnectResult, and CBlockIndexWorkComparator.
The CChainState interface needs to be known to the rest of the system because
many global functions will move to CChainState methods. This is to allow
other parts of the system to be parameterized per chainstate instance
instead of assuming a single global.
1b05dff080 Fix portability issue with pthreads (grim-trigger)
Pull request description:
This change resolves the following issue:
https://github.com/bitcoin/bitcoin/issues/15951
Only tested on OpenBSD 6.5/amd64
ACKs for commit 1b05df:
fanquake:
tACK 1b05dff. Tested on OpenBSD6.4 (`vagrant`).
laanwj:
utACK 1b05dff080
Tree-SHA512: af48581af32820d5adc9ae5abb44f8f1b592c323f86fe2484108b81629389f6ef347598f9a087aa6476ac553e59828cd7927bb4ab11dc70e7c9a944a92fc54ae
The original ORCHID prefix was deprecated as of 2014-03, the new
ORCHIDv2 prefix was allocated by RFC7343 as of 2014-07. We did not
consider the original ORCHID prefix routable, and I don't see any reason
to consider the new one to be either.
f1a77b0c51 [docs] Add doxygen comment for CReserveKey (John Newbery)
37796b2dd4 [docs] Add doxygen comment for CKeyPool (John Newbery)
ef2d515af3 [wallet] move-only: move CReserveKey to be next to CKeyPool (John Newbery)
Pull request description:
Docs/move-only
Adds doxygen comments for the CKeyPool and CReserveKey objects. The way these work is pretty confusing and it's easy to overlook details (eg https://github.com/bitcoin/bitcoin/pull/15557#discussion_r271956393).
These are on the verbose side, but I think too much commenting is better than not enough. Happy to take feedback on what's an appropriate level.
ACKs for commit f1a77b:
jonatack:
Thanks, John. Re-ACK f1a77b0c51, doc-only changes with respect to previous review.
jb55:
ACK f1a77b0c51
Tree-SHA512: 8bc97c7029cd2e8d9bfd2d2144eeff73474c71eda5a9d10817e1578ca0b70da677252037d83143faaff1808e2193408a21a8a89d36049eac77fd313990f0b67b
f6bb11fd37 Add test for ArgsManager::GetChainName (Russell Yanofsky)
4b331159df Add unit test NextString, ForEachNoDup functions (Russell Yanofsky)
05bfee3451 util_SettingsMerge test cleanup (Russell Yanofsky)
Pull request description:
There was some test coverage previously, but it was limited and didn't test conflicting and negated arguments.
ACKs for commit f6bb11:
MarcoFalke:
re-utACK f6bb11fd37
Tree-SHA512: d03596614dc48584c7a9440117b107c6abb23fd4c7fa15fb4015351ec3de08b2656bc956ce05310663675672343d7a6aff35421657f29172080c7005045680b0
ccc27bdcd2 doc: Clarify -blocksdir usage (Daniel McNally)
Pull request description:
This PR attempts to clarify and correct the `-blocksdir` argument description and default value. `-blocksdir` does not refer to the full path to the actual `blocks` directory, but rather the root/parent directory which contains the `blocks` directory. Accordingly, the default value is `<datadir>` and not `<datadir>/blocks` - this behavior of defaulting to the datadir can also be seen in init.cpp:
```cpp
if (gArgs.IsArgSet("-blocksdir")) {
path = fs::system_complete(gArgs.GetArg("-blocksdir", ""));
if (!fs::is_directory(path)) {
path = "";
return path;
}
} else {
path = GetDataDir(false);
}
```
It also attempts to clarify that only the `.dat` files containing block data are impacted by `-blocksdir`, not the index files.
I believe this would close#12828.
ACKs for commit ccc27b:
hebasto:
utACK ccc27bdcd2
Tree-SHA512: 7b65f66b0579fd56e8c8cd4f9f22d6af56181817762a68deccd7fca51820ad82d9a0c48f5f1f012e746c67bcdae7af4555fad867cb620a9ca538d465c9d86c2b
This adds a descriptors argument to the utxoupdatepsbt RPC. This means:
* Input and output scripts and keys will be filled in when known
* P2SH-witness outputs will be filled in from the UTXO set when a descriptor
is provided to show they're segwit outputs.
This is not a pure refactor; additional functionality is added in
IsSegWitOutput which lets it recurse into P2SH when a
SigningProvider is provided that knows about the inner script.
d20d756752 rpc: faster getblockstats using BlockUndo data (Felix Weis)
Pull request description:
Using undo data for a block (rev?????.dat) we can retrieve value information about prevouts and calculate the final transaction fee (rate). This approach is about 80x faster, drops the requirement for `-txindex`, and works for all non-pruned blocks.
```
# 2018-11-25T16:36:19Z Bitcoin Core version v0.17.99.0-edc715240-dirty (release build)
seq 550100 550200 0.00s user 0.00s system 62% cpu 0.004 total
xargs -n1 src/bitcoin-cli getblockstats 0.21s user 0.19s system 17% cpu 2.302 total
# 2018-11-25T16:39:17Z Bitcoin Core version v0.17.0 (release build)
seq 550100 550200 0.00s user 0.00s system 87% cpu 0.002 total
xargs -n1 src/bitcoin-cli getblockstats 0.24s user 0.22s system 0% cpu 3:19.42 total
```
ACKs for commit d20d75:
MarcoFalke:
re-utACK d20d756752
Tree-SHA512: 5babc3eb8d2fee2cb23dc12f522656b80737a540cbf2b13390a8f388304c46c064cca76f896b46a6e2abae8cc582d28e1ab20dd4bb17ad6142f20630c2d30c54
Using undo data for a block (rev?????.dat) we can retrieve value information about prevouts and calculate the final transaction fee (rate). This approach is about 80x faster, drops the requirement for -txindex, and works for all non-pruned blocks.
510c6532ba Extract ParseDescriptorRange (Ben Woosley)
Pull request description:
So as to be consistently informative when the checks fail, and
to protect against unintentional divergence among the checks.
ACKs for commit 510c65:
meshcollider:
Oh apologies, yes. Thanks :) utACK 510c6532ba
MarcoFalke:
utACK 510c6532ba
sipa:
utACK 510c6532ba
Tree-SHA512: b1f0792bfaa163890a20654a0fc2c4c4a996659916bf5f4a495662436b39326692a1a0c825caafd859e48c05f5dd1865c4f7c28092be5074edda3c94f94f9f8b
2dfe27517 Add ChaCha20 bench (Jonas Schnelli)
2bc2b8b49 Add ChaCha20 encryption option (XOR) (Jonas Schnelli)
Pull request description:
The current ChaCha20 implementation does not support message encryption (it can only output the keystream which is sufficient for the RNG).
This PR adds the actual XORing of the `plaintext` with the `keystream` in order to return the desired `ciphertext`.
Required for v2 message transport protocol.
ACKs for commit 2dfe27:
jnewbery:
Looks good. utACK 2dfe275171.
jnewbery:
utACK 2dfe275171
sipa:
utACK 2dfe275171
ryanofsky:
utACK 2dfe275171. Changes since last review are just renaming the Crypt method, adding comments, and simplifying the benchmark.
Tree-SHA512: 84bb234da2ca9fdc44bc29a786d9dd215520f81245270c1aef801ef66b6091b7793e2eb38ad6dbb084925245065c5dce9e5582f2d0fa220ab3e182d43412d5b5
78e407ad0c GetKeyBirthTimes should return key ids, not destinations (Gregory Sanders)
70946e7fee Replace CScriptID and CKeyID in CTxDestination with dedicated types (Gregory Sanders)
Pull request description:
The current usage seems to be an overloading of meanings. `CScriptID` is used in the wallet as a lookup key, as well as a destination, and `CKeyID` likewise. Instead, have all destinations be dedicated types.
New types:
`CScriptID`->`ScriptHash`
`CKeyID`->`PKHash`
ACKs for commit 78e407:
ryanofsky:
utACK 78e407ad0c. Only changes are removing extra CScriptID()s and fixing the test case.
Sjors:
utACK 78e407a
meshcollider:
utACK 78e407ad0c
Tree-SHA512: 437f59fc3afb83a40540da3351507aef5aed44e3a7f15b01ddad6226854edeee762ff0b0ef336fe3654c4cd99a205cef175211de8b639abe1130c8a6313337b9
e0bb279999 Doc: remove text about txes always relayed from -whitelist (David A. Harding)
Pull request description:
Updates text since -whitelistforcerelay was set to false by default in PR #15193.
ACKs for commit e0bb27:
fanquake:
utACK e0bb279
MarcoFalke:
utACK e0bb279999
Tree-SHA512: cf0c9321d72692d573039a04f8f1d048cbdf67ed86cc781523dabd3c45d2731b788f53749e6bb29d7da1ab44eb04030f352469b20489bb2a26c2c38fb61f6489
62d50ef308 Add LOCKS_EXCLUDED(cs_main) to LimitValidationInterfaceQueue(...) which does AssertLockNotHeld(cs_main) (practicalswift)
Pull request description:
This PR adds compile-time checking for negative locking requirements that follow from the run-time locking requirement `AssertLockNotHeld(cs_main)` in `LimitValidationInterfaceQueue(...)`.
Changes:
* Add `LOCKS_EXCLUDED(cs_main)` to `LimitValidationInterfaceQueue(...)` which does `AssertLockNotHeld(cs_main)`
* Add `LOCKS_EXCLUDED(cs_main)` to `CChainState::ActivateBestChain(…)`, `CChainState:: InvalidateBlock(…)` and `CChainState::RewindBlockIndex(…)` which all call `LimitValidationInterfaceQueue(...)` which does `AssertLockNotHeld(cs_main)`
* Add `LOCKS_EXCLUDED(cs_main)` to `InvalidateBlock(…)` which calls `CChainState::InvalidateBlock(...)` which in turn calls `LimitValidationInterfaceQueue(...)` which does `AssertLockNotHeld(cs_main)`
* Add `LOCKS_EXCLUDED(cs_main)` to `RewindBlockIndex(…)` which calls `CChainState::RewindBlockIndex(...)` which in turn calls `LimitValidationInterfaceQueue(...)` which does `AssertLockNotHeld(cs_main)`
ACKs for commit 62d50e:
MarcoFalke:
utACK 62d50ef308
Tree-SHA512: 73d092ccd08c851ae3c5d60370c369fc030c5793f5507e2faccb6f91c851ddc0ce059fbea3899f2856330d7a8c78f2ac6a2988e8268b03154f946be9e60e3be1
Add LOCKS_EXCLUDED(cs_main) to functions calling LimitValidationInterfaceQueue(...) which does AssertLockNotHeld(cs_main)
Add LOCKS_EXCLUDED(cs_main) to functions calling CChainState::InvalidateBlock(...) which calls LimitValidationInterfaceQueue(...) which in turn does AssertLockNotHeld(cs_main)
Add LOCKS_EXCLUDED(cs_main) to functions calling CChainState::RewindBlockIndex(...) which calls LimitValidationInterfaceQueue(...) which in turn does AssertLockNotHeld(cs_main)
486c1eea86 refactoring: remove unused chainActive (James O'Beirne)
631940aab2 scripted-diff: replace chainActive -> ::ChainActive() (James O'Beirne)
a3a609079c refactoring: introduce unused ChainActive() (James O'Beirne)
1b6e6fcfd2 rename: CChainState.chainActive -> m_chain (James O'Beirne)
Pull request description:
This is part of the assumeutxo project:
Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/2019-04-proposal/proposal
---
This change refactors the `chainActive` reference into a `::ChainActive()` call. It also distinguishes `CChainState`'s `CChain` data member as `m_chain` instead of the current `chainActive`, which makes it easily confused with the global data.
The active chain must be obtained via function because its reference will be swapped at some point during runtime after loading a UTXO snapshot.
This change, though lengthy, should be pretty easy to review since most of it is contained within a scripted-diff. Once merged, the parent PR should be easier to review.
ACKs for commit 486c1e:
Sjors:
utACK 486c1ee
promag:
utACK 486c1ee.
practicalswift:
utACK 486c1eea86
Tree-SHA512: 06ed8f9e77f2d25fc9bea0ba86436d80dbbce90a1e8be23e37ec4eeb26060483e60b4a5c4fba679cb1867f61e3921c24abeb9cabdfb4d0a9b1c4ddd77b17456a
Remove testcase generating code from util_SettingsMerge so it can be reused in
new tests.
The hash value expected in util_SettingsMerge changes as a result of this, but
only because the testcases are generated in a different order, not because any
cases are added or removed. It is possible to verify this with:
SETTINGS_MERGE_TEST_OUT=new.txt test/test_bitcoin --run_test=util_tests/util_SettingsMerge
git checkout HEAD~1
make test/test_bitcoin
SETTINGS_MERGE_TEST_OUT=old.txt test/test_bitcoin --run_test=util_tests/util_SettingsMerge
diff -u <(sort old.txt) <(sort new.txt)
The new output is a little more readable, with simpler testcases sorted first.
Followup to #15869. Treat "-wallet" as the network-specific argument in test
instead of "-server", to make test output clearer and be more consistent with
bitcoind. Update embedded hash to match changed output from this.
d2eee87928 Lift prevector default vals to the member declaration (Ben Woosley)
Pull request description:
I overlooked this possibility in #14028
ACKs for commit d2eee8:
promag:
utACK d2eee87, change looks good because members are always initialized.
251Labs:
utACK d2eee87 nice one.
ken2812221:
utACK d2eee87928
practicalswift:
utACK d2eee87928
scravy:
utACK d2eee87928
Tree-SHA512: f2726bae1cf892fd680cf8571027bcdc2e42ba567eaa901fb5fb5423b4d11b29e745e0163d82cb513d8c81399cc85933a16ed66d4a30829382d4721ffc41dc97
b6c748f849 doc: Add release notes for 15730 (João Barbosa)
d3e8458365 rpc: Show scanning details in getwalletinfo (João Barbosa)
90e27abe37 wallet: Track current scanning progress (João Barbosa)
2ee811e693 wallet: Track scanning duration (João Barbosa)
Pull request description:
Closes#15724.
ACKs for commit b6c748:
MarcoFalke:
re-utACK b6c748f849 (Only change since my last review is rebase, adding release notes, and returning false instead of null)
laanwj:
utACK b6c748f849
jonatack:
ACK b6c748f849, only changes appear to be rebase for https://github.com/bitcoin/bitcoin/pull/15730#discussion_r280030617 and release notes.
Tree-SHA512: 8ee98f971c15f66ce8138fc92c55e51abc9faf01866a31ac7ce2ad766aa2bb88559eabee3b5815d645c84cdf1c19dc35ec03f31461e39bc5f6040edec0b87116
facfb4111d rpc: Deprecate getunconfirmedbalance and getwalletinfo balances (MarcoFalke)
999931cf8f rpc: Add getbalances RPC (MarcoFalke)
fad13e925e rpcwallet: Make helper methods const on CWallet (MarcoFalke)
fad40ec915 wallet: Use IsValidNumArgs in getwalletinfo rpc (MarcoFalke)
Pull request description:
This exposes the `CWallet::GetBalance()` struct over RPC.
In the future, incorrectly named rpcs such as `getunconfirmedbalance` or rpcs redundant to this such as `getbalance` could be removed.
ACKs for commit facfb4:
jnewbery:
utACK facfb4111d
Tree-SHA512: 1f54fedce55df9a8ea82d2b6265354b39a956072621876ebaee2355aac0e23c7b64340c3279502415598c095858529e18b50789be956250aafda1cd3a8d948a5
fc929842c2 GUI: Move QRImageWidget to its own file-pair (Luke Dashjr)
77851ab682 GUI: Refactor actual QR code rendering into new QRImageWidget::setQR (Luke Dashjr)
Pull request description:
For at least QR-code based pairing of mobile wallets with nodes, it will be desirable to render QR codes even without wallet support.
Therefore, this prepares by moving the QRImageWidget out of a wallet-specific file into its own `qrencoder` file-pair.
ACKs for commit fc9298:
laanwj:
utACK fc929842c2
jonasschnelli:
utACK fc929842c2
Tree-SHA512: 95529a38c0573a4b3f1253fb5f11ca07a5b3a9840ec24acc7d87270212f3c9f7c5b186d9274d297517a3b80494f38a57574fb9730b1574db01688539b987bd91
0ff1c2a838 Separate reason for premature spends (coinbase/locktime) (Suhas Daftuar)
54470e767b Assert validation reasons are contextually correct (Suhas Daftuar)
2120c31521 [refactor] Update some comments in validation.cpp as we arent doing DoS there (Matt Corallo)
12dbdd7a41 [refactor] Drop unused state.DoS(), state.GetDoS(), state.CorruptionPossible() (Matt Corallo)
aa502b88d1 scripted-diff: Remove DoS calls to CValidationState (Matt Corallo)
7721ad64f4 [refactor] Prep for scripted-diff by removing some \ns which annoy sed. (Matt Corallo)
5e78c5734b Allow use of state.Invalid() for all reasons (Matt Corallo)
6b34bc6b6f Fix handling of invalid headers (Suhas Daftuar)
ef54b486d5 [refactor] Use Reasons directly instead of DoS codes (Matt Corallo)
9ab2a0412e CorruptionPossible -> BLOCK_MUTATED (Matt Corallo)
6e55b292b0 CorruptionPossible -> TX_WITNESS_MUTATED (Matt Corallo)
7df16e70e6 LookupBlockIndex -> CACHED_INVALID (Matt Corallo)
c8b0d22698 [refactor] Drop redundant nDoS, corruptionPossible, SetCorruptionPossible (Matt Corallo)
34477ccd39 [refactor] Add useful-for-dos "reason" field to CValidationState (Matt Corallo)
6a7f8777a0 Ban all peers for all block script failures (Suhas Daftuar)
7b999103e2 Clean up banning levels (Matt Corallo)
b8b4c80146 [refactor] drop IsInvalid(nDoSOut) (Matt Corallo)
8818729013 [refactor] Refactor misbehavior ban decisions to MaybePunishNode() (Matt Corallo)
00e11e61c0 [refactor] rename stateDummy -> orphan_state (Matt Corallo)
f34fa719cf Drop obsolete sigops comment (Matt Corallo)
Pull request description:
This is a rebase of #11639 with some fixes for the last few comments which were not yet addressed.
The original PR text, with some strikethroughs of text that is no longer correct:
> This cleans up an old main-carryover - it made sense that main could decide what DoS scores to assign things because the DoS scores were handled in a different part of main, but now validation is telling net_processing what DoS scores to assign to different things, which is utter nonsense. Instead, we replace CValidationState's nDoS and CorruptionPossible with a general ValidationInvalidReason, which net_processing can handle as it sees fit. I keep the behavior changes here to a minimum, but in the future we can utilize these changes for other smarter behavior, such as disconnecting/preferring to rotate outbound peers based on them providing things which are invalid due to SOFT_FORK because we shouldn't ban for such cases.
>
> This is somewhat complementary with, though obviously conflicts heavily with #11523, which added enums in place of DoS scores, as well as a few other cleanups (which are still relevant).
>
> Compared with previous bans, the following changes are made:
>
> Txn with empty vin/vout or null prevouts move from 10 DoS
> points to 100.
> Loose transactions with a dependency loop now result in a ban
> instead of 10 DoS points.
> ~~BIP68-violation no longer results in a ban as it is SOFT_FORK.~~
> ~~Non-SegWit SigOp violation no longer results in a ban as it
> considers P2SH sigops and is thus SOFT_FORK.~~
> ~~Any script violation in a block no longer results in a ban as
> it may be the result of a SOFT_FORK. This should likely be
> fixed in the future by differentiating between them.~~
> Proof of work failure moves from 50 DoS points to a ban.
> Blocks with timestamps under MTP now result in a ban, blocks
> too far in the future continue to not result in a ban.
> Inclusion of non-final transactions in a block now results in a
> ban instead of 10 DoS points.
Note: The change to ban all peers for consensus violations is actually NOT the change I'd like to make -- I'd prefer to only ban outbound peers in those situations. The current behavior is a bit of a mess, however, and so in the interests of advancing this PR I tried to keep the changes to a minimum. I plan to revisit the behavior in a followup PR.
EDIT: One reviewer suggested I add some additional context for this PR:
> The goal of this work was to make net_processing aware of the actual reasons for validation failures, rather than just deal with opaque numbers instructing it to do something.
>
> In the future, I'd like to make it so that we use more context to decide how to punish a peer. One example is to differentiate inbound and outbound peer misbehaviors. Another potential example is if we'd treat RECENT_CONSENSUS_CHANGE failures differently (ie after the next consensus change is implemented), and perhaps again we'd want to treat some peers differently than others.
ACKs for commit 0ff1c2:
jnewbery:
utACK 0ff1c2a838
ryanofsky:
utACK 0ff1c2a838. Only change is dropping the first commit (f3883a321bf4ab289edcd9754b12cae3a648b175), and dropping the temporary `assert(level == GetDoS())` that was in 35ee77f2832eaffce30042e00785c310c5540cdc (now c8b0d22698)
Tree-SHA512: e915a411100876398af5463d0a885920e44d473467bb6af991ef2e8f2681db6c1209bb60f848bd154be72d460f039b5653df20a6840352c5f7ea5486d9f777a3
Though at the moment ChainActive() simply references `g_chainstate.m_chain`,
doing this change now clears the way for multiple chainstate usage and allows
us to script the diff.
-BEGIN VERIFY SCRIPT-
git grep -l "chainActive" | grep -E '(h|cpp)$' | xargs sed -i '/chainActive =/b; /extern CChain& chainActive/b; s/\(::\)\{0,1\}chainActive/::ChainActive()/g'
-END VERIFY SCRIPT-
This can't be a scripted-diff due to the confusion of the global
chainActive and the CChainState member of the same name.
This specific rename makes the following chainActive -> ::ChainActive() diff
scriptable.
faea56400d rpc: Add lock annotations to block{,header}ToJSON (MarcoFalke)
fab00a5cb9 rpc: Serialize in getblock without cs_main (MarcoFalke)
fa1c3591ad rpc: Use IsValidNumArgs in getblock (MarcoFalke)
Pull request description:
No need to hold cs_main when serializing a struct to json
Fixes: #15925
ACKs for commit faea56:
jnewbery:
utACK faea56400d
jonasschnelli:
utACK faea56400d
Tree-SHA512: 005d378cda1e6024e9f5142f99a8adbefe202cd7bfeaafee55eb909e8990a3790aa27fcf5dd16119cc9afe9dc8bd30f660de40233316781669be166bac3018e7
This is a first step towards cleaning up our DoS interface - make
validation return *why* something is invalid, and let net_processing
figure out what that implies in terms of banning/disconnection/etc.
Behavior change: peers will now be banned for providing blocks
with premature coinbase spends.
Co-authored-by: Anthony Towns <aj@erisian.com.au>
Suhas Daftuar <sdaftuar@gmail.com>
Compared with previous bans, the following changes are made:
* Txn with empty vin/vout or null prevouts move from 10 DoS
points to 100.
* Loose transactions with a dependency loop now result in a ban
instead of 10 DoS points.
* Many pre-segwit soft-fork errors now result in a ban.
Note: Transactions that violate soft-fork script flags since P2SH do not generally
result in a ban. Also, banning behavior for invalid blocks is dependent on
whether the node is validating with multiple script check threads, due to a long-
standing bug. That inconsistency is still present after this commit.
* Proof of work failure moves from 50 DoS points to a ban.
* Blocks with timestamps under MTP now result in a ban, blocks
too far in the future continue to *not* result in a ban.
* Inclusion of non-final transactions in a block now results in a
ban instead of 10 DoS points.
Co-authored-by: Anthony Towns <aj@erisian.com.au>
Isolate the decision of whether to ban a peer to one place in the
code, rather than having it sprinkled throughout net_processing.
Co-authored-by: Anthony Towns <aj@erisian.com.au>
Suhas Daftuar <sdaftuar@gmail.com>
John Newbery <john@johnnewbery.com>
This comment was confusing and incorrect when first added ("invalid rather than
merely non-standard" has the opposite meaning of what is actually the case),
and was also not updated after segwit with the correct variable names.
Delete it since the code reads just fine on its own.
Co-authored by: Anthony Towns <aj@erisian.com.au>
Suhas Daftuar <sdaftuar@gmail.com>
beb42d71a0 Silence GCC 7 warning "control reaches end of non-void function" (-Wreturn-type) in psbt.cpp (practicalswift)
Pull request description:
Silence GCC 7 warning "control reaches end of non-void function" (`-Wreturn-type`) in `psbt.cpp`.
Context: ef22fe8c1f (r33370109)
Before this patch:
```
$ ./configure CC=gcc-7 CXX=g++-7
$ make 2>&1 | grep -A2 "warning: "
leveldb/util/logging.cc:58:40: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
(v == kMaxUint64/10 && delta > kMaxUint64%10)) {
~~~~~~^~~~~~~~~~~~~~~
--
leveldb/port/port_posix.cc:60:15: warning: ‘ecx’ may be used uninitialized in this function [-Wmaybe-uninitialized]
return (ecx & (1 << 20)) != 0;
~~~~~^~~~~~~~~~~~
--
psbt.cpp:341:1: warning: control reaches end of non-void function [-Wreturn-type]
}
^
$
```
After this patch:
```
$ ./configure CC=gcc-7 CXX=g++-7
$ make 2>&1 | grep -A2 "warning: "
leveldb/util/logging.cc:58:40: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
(v == kMaxUint64/10 && delta > kMaxUint64%10)) {
~~~~~~^~~~~~~~~~~~~~~
--
leveldb/port/port_posix.cc:60:15: warning: ‘ecx’ may be used uninitialized in this function [-Wmaybe-uninitialized]
return (ecx & (1 << 20)) != 0;
~~~~~^~~~~~~~~~~~
$
```
ACKs for commit beb42d:
Tree-SHA512: b068b9aef565cae0bd1fa1f79c8d422ed2c3e7645edfa14a780a36dd66095a3c627f4111a6b16e706ce6c8abafe51725af8b3bf60778821de0aa8f6193bfadf8
5d35ae3326 Handle the result of posix_fallocate system call (Luca Venturini)
Pull request description:
The system call `posix_fallocate` is not supported on some filesystems.
- catches the result of posix_allocate and fall back to the default behaviour if the return value is different from 0 (success)
Fixes#15624
ACKs for commit 5d35ae:
MarcoFalke:
utACK 5d35ae3326
sipa:
utACK 5d35ae3326, though the Yoda condition is an uncommon style in this project.
hebasto:
utACK 5d35ae3326
practicalswift:
utACK 5d35ae3326
Tree-SHA512: 7ab3b35fb633926f28a58b2b07ffde8e31bb997c80a716b1b45ee716fe9ff4ddcef0a05810bd4423530e220cfc62f8925517d27a8b92b05a524272063e43f746
422677963a refactor: replace isPotentialtip/waitForNotifications by higher method (Antoine Riard)
edfe9438ca Add WITH_LOCK macro: run code while locking a mutex (Antoine Riard)
Pull request description:
In Chain interface, instead of a isPotentialTip and a WaitForNotifications method, both used only once in CWallet::BlockUntilSyncedToCurrentChain, combine them in a higher WaitForNotificationsUpToTip method. Semantic should be unchanged, wallet wait for pending notifications to be processed unless block hash points to the current chain tip or a descendant.
ACKs for commit 422677:
jnewbery:
ACK 422677963a
ryanofsky:
utACK 422677963a. Only change is adding the cs_wallet lock annotation.
Tree-SHA512: 2834ff0218795ef607543fae822e5cce25d759c1a9cfcb1f896a4af03071faed5276fbe0966e0c6ed65dc0e88af161899c5b2ca358a2d24fe70969a550000bf2
effe81f750 Move g_is_mempool_loaded into CTxMemPool::m_is_loaded (Ben Woosley)
bb8ae2c419 rpc: Expose g_is_mempool_loaded via getmempoolinfo and /rest/mempool/info.json (Ben Woosley)
Pull request description:
And use it to fix a race condition in mempool_persist.py:
https://travis-ci.org/Empact/bitcoin/jobs/487577243
Since e.g. getrawmempool returns errors based on this status, this
enables users to test it for readiness.
Fixes#12863
ACKs for commit effe81:
MarcoFalke:
utACK effe81f750
jnewbery:
utACK effe81f750
Tree-SHA512: 74328b0c17a97efb8a000d4ee49b9a673c2b6dde7ea30c43a6a2eff961a233351c9471f9a42344412135786c02bdf2ee1b2526651bb8fed68bd94d2120c4ef86
151f3e9cf1 Add settings merge test to prevent regresssions (Russell Yanofsky)
Pull request description:
Test-only change. Motivation: I'm trying to clean up settings code and add support for read/write settings without changing existing behavior, but current tests are very scattershot and don't actually cover a lot of current behavior.
ACKs for commit 151f3e:
jonasschnelli:
utACK 151f3e9cf1.
MarcoFalke:
utACK 151f3e9cf1
Tree-SHA512: f9062f078da02855cdbdcae37d0cea5684e82adbe5c701a8eb042ee4a57d899f0ffb6a9db3bcf58b639dff22b2b2d8a75f9a7917402df58904036753d65a1e3e
Boost Filesystem basename() function is deprecated since v1.36.0.
Also, defining BOOST_FILESYSTEM_NO_DEPRECATED before including
filesystem headers is strongly recommended. This prevents inadvertent
use of old features, particularly legacy function names, that have been
replaced and are going to go away in the future.
This work is prerequisite to attaching thread names to log lines and deadlock
debug utilities. This code allows setting of an "internal" threadname per
thread on platforms where thread_local is available.
This commit also moves RenameThread() out of a more general module and adds a
numeric suffix to disambiguate between threads with the same name. It
explicitly names a few main threads using the new util::ThreadRename().
765d5890be Bugfix: dummywallet: Add -ignorepartialspends to list of ignored wallet options (Luke Dashjr)
0f09eb779d dummywallet: Reformat ignored wallet options list (Luke Dashjr)
Pull request description:
When building w/o wallet support, we add all the wallet options as hidden options to avoid throwing errors/warnings that they're unknown.
`-ignorepartialspends` is missing from this list. This PR adds it.
(This seems like a good candidate for a linter? Or maybe we can autogenerate it?)
Also reformats the dummywallet options list across multiple lines to make conflicts less often.
ACKs for commit 765d58:
meshcollider:
utACK 765d5890be
MarcoFalke:
utACK 765d5890be
promag:
utACK 765d589.
practicalswift:
utACK 765d5890be
fanquake:
utACK 765d589
Tree-SHA512: 37c8037148bdc1b7a8bde201eff51ee6a64c042c17eb8b6c68faef490d16575348c2f22ab81f48302b8ad80a5559222af23b721a8b5acc1d89c0757fb88796a6
70c1cf8c1c wallet: Avoid logging no_such_file_or_directory error (João Barbosa)
Pull request description:
Avoid logging `IsBerkeleyBtree: No such file or directory ...`. The result of `IsBerkeleyBtree` is the same since `fs::file_size()` returns 0 for non existent files.
Fix#15912.
ACKs for commit 70c1cf:
practicalswift:
utACK 70c1cf8c1c
jonasschnelli:
utACK 70c1cf8c1c
Empact:
utACK 70c1cf8c1c
Tree-SHA512: 964a64fff9a17b805a1570884cdb2beb82283498f790b0464e011791803ae7f37dba213320c76a920dd92b2b972a7640e6277ecf470400734149dc8f9f9f1d6d
5c759c73b2 [wallet] Move maxTxFee to wallet (John Newbery)
Pull request description:
Closes#15355
Moves the `-maxtxfee` from the node to the wallet. See discussion in issue for details.
This is a cleanup. There is no change in behaviour.
Completes #15620
ACKs for commit 5c759c:
MarcoFalke:
utACK 5c759c73b2
ryanofsky:
utACK 5c759c73b2. Changes since last review: updated commit message and an error message and method name.
meshcollider:
utACK 5c759c73b2
Tree-SHA512: 2f9b2729da3940a5cda994d3f3bc11ee1a52fcc1c5e9842ea0ea63e4eb0300e8416853046776311298bc449ba07554aa46f0f245ce28598a5b0bd7347c12e752
c634b1e20 [POLICY] Make sending to future native witness outputs standard (Pieter Wuille)
Pull request description:
As discussed in the April 18 2019 IRC meeting.
This makes sending to future Segwit versions via native outputs (bech32) standard for relay, mempool acceptance, and mining. The reasons are:
* This may interfere with smooth adoption of future segwit versions, if they're defined (by the sender wallet/node).
* It violates BIP173 ("Version 0 witness addresses are always 42 or 62 characters, but implementations MUST allow the use of any version."), though admittedly this code was written before BIP173.
* It doesn't protect much, as P2SH-embedded segwit cannot be filtered in this way.
* As a general policy, the sender shouldn't care what the receiver likes his outputs to be.
Note that _spending_ such outputs (including P2SH-embedded ones) remains nonstandard, as that is actually required for softfork safety.
ACKs for commit c634b1:
MarcoFalke:
utACK c634b1e207
harding:
Tested ACK c634b1e207
meshcollider:
utACK c634b1e207
Tree-SHA512: e37168a1be9f445a04d4280593f0a92bdae33eee00ecd803d5eb16acb5c9cfc0f1f0a1dfbd5a0cc73da2c9928ec11cbdac7911513a78f85b789ae0d00e1b5962
99e88a372 rpc: Remove dependency on interfaces::Chain in SignTransaction (Antoine Riard)
Pull request description:
Assuming wallet RPCs and node RPCs will go into different processes, signrawtransactionwithkey doesn't need to access Coins via interfaces::Chain, it may use directly utility in node/coins.cpp
Obviously will need rebase after #15638
Tree-SHA512: 42ee8fcbcd38643bbd82210db6f68249bed5ee036a4c930a1db534d0469a133e287b8869c977bf0cc79a7296dde04f72adb74d24e1cd20f4a280f4c2b7fceb74
5d26205272 doc: explain AcceptToMemoryPoolWorker's coins_to_uncache (James O'Beirne)
Pull request description:
I found ATMPW's `coins_to_uncache` a little hard to understand (see #15264). This adds some doc for posterity.
ACKs for commit 5d2620:
jnewbery:
ACK 5d26205272
Tree-SHA512: 088508fa78012fab8680663c4e30f5cee29768416c2ca8b8b2abc29b6ac7067c5a589674f0254474a7ccc95477889d41719760f5796792bf492f51b3dd499c6c
This prevents a bug where the in-flight queue for our peers will not be
drained, resulting in not downloading any new transactions from our peers.
Thanks to ajtowns for reporting this bug.
Previously there was an implicit bound based on the handling of m_tx_announced,
but that approach is error-prone (particularly if we start automatically
removing things from that set).
bb530efa18 Disallow extended encoding for non-witness transactions (Pieter Wuille)
Pull request description:
BIP144 specifies that transactions without witness should use the legacy encoding, which is currently not enforced.
This rule was present in the original SegWit implementation (https://github.com/bitcoin/bitcoin/pull/8149), but was subsequently dropped (https://github.com/bitcoin/bitcoin/pull/8589).
As all hashes, txids, and weights are always computed over a reserialized version of a transaction, it is mostly harmless to permit extended encoding for non-segwit transactions, but I'd rather strictly follow the BIP.
ACKs for commit bb530e:
instagibbs:
utACK bb530efa18
stevenroose:
utACK bb530efa18
Tree-SHA512: 1aeccd6a555f43784fefb076ce2e8ad2f5ba7be49840544a50050d0390f82373f87201bf56cf8bb30841b4f9cd893b382261a080da875d4e11ab7051f8640dbe
fa1c8e297 Resolve the qt/guiutil <-> qt/optionsmodal CD (251)
Pull request description:
This pull request attempts to resolve the `qt/guiutil` <-> `qt/optionsmodel` circular dependency.
The `Intro` class in `qt/intro` has a static member function `getDefaultDataDirectory` which is used by `qt/optionsmodel` and creates the circular dependency
`qt/guiutil -> qt/walletmodel -> qt/optionsmodel -> qt/intro -> qt/guiutil`.
This circular dependency is resolved by moving `Intro::getDefaultDataDirectory` to `GUIUtil::getDefaultDataDirectory` without modifying the implementation.
ACKs for commit fa1c8e:
MarcoFalke:
utACK fa1c8e2978
promag:
utACK fa1c8e2.
hebasto:
utACK fa1c8e2978
practicalswift:
utACK fa1c8e2978
jonasschnelli:
utACK fa1c8e2978
Tree-SHA512: 58cc4aee937c943d8de9dc97ef1789decfddb0287308f44e7e3a3b497c19e51da184988e17207544fff410168ec98dd49a3e62c47e84ad1f0cf6ef7247a80fb5
710a7136f9 rpc: Speedup getaddressesbylabel (João Barbosa)
Pull request description:
Fixes#15447. Same approach of #14984, this change avoids duplicate key check when building the JSON response in memory.
ACKs for commit 710a71:
MarcoFalke:
utACK 710a7136f9
ryanofsky:
utACK 710a7136f9. Just new comments and assert since last review.
Tree-SHA512: 77c95df9ff3793e348619aa070e6fd36df9da1b461d708ab146652cb3699f1a472ef6eb38dafdb8374375cbc97daef07635fcb0501961f167a023309513742e2
c9e6e7ed7 wallet: add cachable amounts for caching credit/debit values (Karl-Johan Alm)
Pull request description:
This is a refactoring that will make #13756 a lot cleaner and straight-forward, since it adds another combination to the pile (watch-only * spendable * reused).
It's also a nice change in general.
Tree-SHA512: 6c876d58bbffd5cb85ef632dea4fd6afed163904bbde5efdb307fa119af178ed3cb5df047255da7e9a9136fed876922f1116fce61a3710f308c72275f9b7d18b
This pull request attempts to resolve the `qt/guiutil` <-> `qt/optionsmodel`
circular dependency.
The circular dependency is resolved by moving the `Intro::getDefaultDataDirectory`
member function to `GUIUtil::getDefaultDataDirectory`.
