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
This commit adds comments referencing multiple CVEs both in production and test code.
CVEs covered in this commit:
CVE-2010-5137
CVE-2010-5139
CVE-2010-5141
CVE-2012-1909
CVE-2012-2459
CVE-2012-3789
CVE-2018-17144
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
