ValidationState::Invalid() takes a parameter `ret` which is returned to
the caller. All call sites set this to false. Remove the `ret` parameter
and just return false always.
This is in preparation for the next commit, which removes the useless
`ret` parameter from ValidationState::Invalid().
error() is simply a convenience wrapper that calls LogPrintf and returns
false. Call LogPrintf explicitly and substitute the error() call for a
false bool literal.
0a433fc876 [validation] Remove unused cacheSigStore from CheckInputsFromMempoolAndCache (John Newbery)
Pull request description:
CheckInputsFromMempoolAndCache() is only called in one place, and
cacheSigStore is set to true in that call site. Remove the argument
entirely.
Also improve commenting.
ACKs for top commit:
MarcoFalke:
unsigned ACK 0a433fc876 Comment looks good
jamesob:
ACK 0a433fc876
laanwj:
ACK 0a433fc876
fanquake:
ACK 0a433fc876. Checked that `CheckInputsFromMempoolAndCache` is only called once, in `MemPoolAccept::ConsensusScriptChecks`, and that `cacheSigStore` is true.
Tree-SHA512: e4b4d2550e35df55c8f8fa4c539174cc2d3728112ddb937cb2ff759d8630a01566b5ec42a70a82e33994e6586f5a457a75a59f64b15d27c65331c723cbb097af
b05ec410f2 Add unit testing for the CompressScript functions (marcaiaf)
Pull request description:
Salvaging #15104 which adds unit tests for CompressScript function in `compressor.cpp`
Tested following cases for the CScript:
- CKeyID
- CScriptID
- Uncompressed CPubKey (of size: 65)
- Compressed CPubKey (of size: 32)
ACKs for top commit:
theStack:
ACK b05ec410f2
Tree-SHA512: 7e23ace39383122802dfe5f7d38190d772f5db4045a67b7a9bd4c06797a17e0cdc41d6fac92d448057eb7df50172155dc824587c16c68c79fd1a4de37b772001
fa92813407 consensus: Explain why fCheckDuplicateInputs can not be skipped and remove it (MarcoFalke)
Pull request description:
As a follow up to CVE-2018-17144, this removes the unused `fCheckDuplicateInputs` parameter and explains why the test can not be disabled. Apart from protecting against a dumb accident in the future, this should document the logic in the code. There is a technical write-up that explains how the underlying coins database behaves if this test is skipped: https://bitcoincore.org/en/2018/09/20/notice/#technical-details. However, it does not explicitly mention why the test can not be skipped. I hope my code comment does that.
ACKs for top commit:
jnewbery:
ACK fa92813407
amitiuttarwar:
utACK fa92813407
Empact:
Code review ACK fa92813407
promag:
ACK fa92813407.
Tree-SHA512: fc1ef670f1a467c543b84f704b9bd8cc7a59a9f707be048bd9b4e85fe70830702aa560a880efa2c840bb43818ab44dfdc611104df04db2ddc14ff92f46bfb28e
50037e97d1 depends: fix boost mac cross build with clang 9+ (Cory Fields)
Pull request description:
The ancient "darwin-4.9.1" profile has long been used to match against clang, which prior to version 9, reported 4.9.1 as its version when invoking "clang++ -dumpversion". Presumably this was a historical compatibility quirk related to Apple's switch from gcc to clang.
This was "fixed" in clang 9.0, so that -dumpversion reports the real version. Unfortunately that had the side-effect of breaking the (brittle) boost compiler detection.
Move to the seemingly more-correct "clang-darwin" profile, which passes the checks and builds correctly.
Also switch to using ar rather than libtool for archiving, as it's what the clang-darwin profile expects to be using.
Note that because this is using a different profile, some of the final command-line arguments end up changing. Those changes look sane at a glance.
ACKs for top commit:
fanquake:
ACK 50037e97d1 - tested on on macOS, will wait for the gitian build.
Tree-SHA512: eac1f353513a445add6fbece7fc78dd3dbdde5e2219bfb7739b82f40bb14de449667a94d2e303d43c67d9b38e7ceb0ba5f0d8fe20b40be2017b1ca0875467c2c
168b781fe7 Continue relaying transactions after they expire from mapRelay (Anthony Towns)
Pull request description:
This change allows peers to request transactions even after they've expired from mapRelay and even if they're not doing mempool requests. This is intended to allow for CPFP of old transactions -- if parent tx P wasn't relayed due to low fees, then a higher fee rate child C is relayed, peers will currently request the parent P, but we prior to this patch, we will not relay it due to it not being in mapRelay.
ACKs for top commit:
MarcoFalke:
re-ACK 168b781fe7 (only change is comment fixup)
sdaftuar:
re-ACK 168b781fe7
sipa:
ACK 168b781fe7
Tree-SHA512: b206666dd1450cd0a161ae55fd1a7eda2c3d226842ba27d91fe463b551fd924b65b92551b14d6786692e15cf9a9a989666550dfc980b48ab0f8d4ca305bc7762
fa710066b9 ci: Disable functional tests on mac host (MarcoFalke)
Pull request description:
Judging from the lack of responses to https://github.com/bitcoin/bitcoin/issues/15400#issuecomment-543745053, no one can reproduce the failures locally. Thus, disable the tests on the ci mac host. Otherwise they cause ci failures to be ignored or overwritten by a blind re-run.
ACKs for top commit:
laanwj:
ok, ACK fa710066b9 in that case
ryanofsky:
Code review ACK fa710066b9. I've been blindly restarting these but can confirm it seemed like random test failures.
Tree-SHA512: 805f552124409f6e79b16b419826d334307202b8e3636edc8a91dadffe9949554d924c00cb16bb09473ec1aaaf2d8eaf7a97c7c2b8c11bf6372135d39bba802b
CheckInputsFromMempoolAndCache() is only called in one place, and
cacheSigStore is set to true in that call site. Remove the argument
entirely.
Also improve commenting.
6f6852335f doc: Changed miniupnp links to https (Marius Kjærstad)
Pull request description:
doc: Changed miniupnp links to https
ACKs for top commit:
fjahr:
ACK 6f68523
fanquake:
ACK 6f6852335f - as mentioned in this [comment](https://github.com/bitcoin/bitcoin/pull/17166#issuecomment-545906845), in future, there's no need to open a new PR instead than squashing. One of the requirements of contributing to Bitcoin Core is some basic `git` usage, and we have some documentation on that in our [CONTRIBUTING.md](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits) (which also mentions not to open multiple PRs).
Tree-SHA512: 1e806326552b94dc1c2f105bed822a1dd56127d42bbb0c15a16623ff084b2047331f2f1ad5303c99b0d68c70ad2caf8e8028eb354ba4379973b9e1198676ce72
c2f964a674 tests: Remove Cygwin WinMain workaround (practicalswift)
db4bd32cc3 tests: Skip unnecessary fuzzer initialisation. Hold ECCVerifyHandle only when needed. (practicalswift)
Pull request description:
Skip unnecessary fuzzer initialisation. Hold `ECCVerifyHandle` only when needed.
As suggested by MarcoFalke in https://github.com/bitcoin/bitcoin/pull/17018#discussion_r336645391.
Top commit has no ACKs.
Tree-SHA512: 598da44859d736e3fdc143b93e07f444d8ad19dfdab0cfe7c6ccff8644e862664d869337dfe6b49416ed09a0024e4a5f2220ca6246de568f9e9227d721baa28e
9075d13153 [docs] Add release notes for removal of REJECT reasons (John Newbery)
04a2f326ec [validation] Fix REJECT message comments (John Newbery)
e9d5a59e34 [validation] Remove REJECT code from CValidationState (John Newbery)
0053e16714 [logging] Don't log REJECT code when transaction is rejected (John Newbery)
a1a07cfe99 [validation] Fix peer punishment for bad blocks (John Newbery)
Pull request description:
We no longer send BIP 61 REJECT messages, so there's no need to set
a REJECT code in the CValidationState object.
Note that there is a minor bug fix in p2p behaviour here. Because the
call to `MaybePunishNode()` in `PeerLogicValidation::BlockChecked()` only
previously happened if the REJECT code was > 0 and < `REJECT_INTERNAL`,
then there are cases were `MaybePunishNode()` can get called where it
wasn't previously:
- when `AcceptBlockHeader()` fails with `CACHED_INVALID`.
- when `AcceptBlockHeader()` fails with `BLOCK_MISSING_PREV`.
Note that `BlockChecked()` cannot fail with an 'internal' reject code. The
only internal reject code was `REJECT_HIGHFEE`, which was only set in
ATMP.
This reverts a minor bug introduced in 5d08c9c579.
ACKs for top commit:
ariard:
ACK 9075d13, changes since last reviewed are splitting them in separate commits to ease understanding and fix nits
fjahr:
ACK 9075d13153, confirmed diff to last review was fixing nits in docs/comments.
ryanofsky:
Code review ACK 9075d13153. Only changes since last review are splitting the main commit and updating comments
Tree-SHA512: 58e8a1a4d4e6f156da5d29fb6ad6a62fc9c594bbfc6432b3252e962d0e9e10149bf3035185dc5320c46c09f3e49662bc2973ec759679c0f3412232087cb8a3a7
9e95931865 [wallet] Remove `state` argument from CWallet::CommitTransaction (John Newbery)
d1734f9a3b [wallet] Remove return value from CommitTransaction() (John Newbery)
b6f486a02b [wallet] Add doxygen comment to CWallet::CommitTransaction() (John Newbery)
8bba91b22d [wallet] Fix whitespace in CWallet::CommitTransaction() (John Newbery)
Pull request description:
`CommitTransaction()` returns a bool to indicate success, but since commit
b3a7410 (#9302) it only returns true, even if the transaction was not
successfully broadcast. This commit changes CommitTransaction() to return
void.
All dead code in `if (!CommitTransaction())` branches has been removed.
Two additional commits fix up the idiosyncratic whitespace in `CommitTransaction` and add a doxygen comment for the function.
ACKs for top commit:
laanwj:
ACK 9e95931865
Tree-SHA512: a55a2c20369a45222fc0e02d0891495655a926e71c4f52cb72624768dd7b9c1dca716ea67d38420afb90f40c6e0fd448caa60c18fd693bb10ecb110b641820e6
4896bacc00 Add testcase to simulate bitcoin schema in leveldb (MapleLaker)
Pull request description:
Resurrecting #14125 with updates based on comments of closed PR
ACKs for top commit:
laanwj:
ACK 4896bacc00
dongcarl:
ACK 4896bacc00
Tree-SHA512: 3290ea7e1e998901d5ee8921d1d76cec399cae30ac1911a45b86826afed47cee1acf92bd6438f1fa11ed785a3b17abdcb1c169bc0419945eda9fe4c089d0b6eb
The ancient "darwin-4.9.1" profile has long been used to match against
clang, which prior to version 9, reported 4.9.1 as its version when
invoking "clang++ -dumpversion". Presumably this was a historical
compatibility quirk related to Apple's switch from gcc to clang.
This was "fixed" in clang 9.0, so that -dumpversion reports the real
version. Unfortunately that had the side-effect of breaking the
(brittle) boost compiler detection.
Move to the seemingly more-correct "clang-darwin" profile, which passes
the checks and builds correctly.
Also switch to using ar rather than libtool for archiving, as it's what
the clang-darwin profile expects to be using.
Note that because this is using a different profile, some of the final
command-line arguments end up changing. The changes look sane at a
glance.
a0fc076476 refactor: test/bench: dedup Build{Crediting,Spending}Transaction() (Sebastian Falbesoner)
Pull request description:
prototypes used in `src/test/script_tests.cpp`:
- `CMutableTransaction BuildCreditingTransaction(const CScript& scriptPubKey, int nValue = 0);`
- `CMutableTransaction BuildSpendingTransaction(const CScript& scriptSig, const CScriptWitness& scriptWitness, const CTransaction& txCredit);`
prototypes used in `bench/verify_script.cpp`:
- `CMutableTransaction BuildCreditingTransaction(const CScript& scriptPubKey);`
- `CMutableTransaction BuildSpendingTransaction(const CScript& scriptSig, const CMutableTransaction& txCredit);`
The more generic versions from the script tests are moved into `setup_common.cpp` and the calls are adapted accordingly in the verify_script benchmark (passing the nValue of 1 explicitely for `BuildCreditingTransaction()`, passing empty scriptWitness explicitely and converting txCredit parameter to CTransaction in `BuildSpendingTransaction()`).
Top commit has no ACKs.
Tree-SHA512: 8444f8a18f15070eeec1e5dfd255b55a851dfc2e6647c12b1995a6f7abd7196e830db2181d0e860bcd4cf4c815967584a3756dd450346bca70649dd1d4493e04
cd68594dcd Only check the hash of transactions loaded from disk (Andrew Chow)
Pull request description:
It feels unnecessary to do a full `CheckTransaction` for every transaction saved in the wallet. It should not be possible for an invalid transaction to get into the wallet in the first place, and if there is any disk corruption, the hash check will catch it.
ACKs for top commit:
MarcoFalke:
ACK cd68594dcd
laanwj:
ACK cd68594dcd
promag:
ACK cd68594dcd, AFAICT the check is not needed, hash comparison gives data integrity.
Tree-SHA512: 5b2e719f76097cfbf125392db6cc6c764355c81f0b7a5b60aee4b06af1afcca80cfd38a3cf5307fd9e2c1afc405f8321929a4552943099a8161e6762965451fb
088a730fe6 static tooltip (JeremyCrookshank)
Pull request description:
I noticed that on Bitcoin sends the tooltip wasn't very clear for new users and I hope my PR is more concise. If it needs changing more will happily change too 👍
ACKs for top commit:
laanwj:
ACK 088a730fe6
Tree-SHA512: 2b1103ac934d8f68d22333af3c0f5d4228b665b1e507378d4ae5b83cc2b6d6aeb46a3d68298cca93feb839db5caa560322c8df5261dc2f7db5abeed9f0dd9c69
89339d1460 tests: Add test for loadblock option (Fabian Jahr)
Pull request description:
Fixes#17019
Was initially part of #17044 but as the test got larger it made sense to split it into its own commit as suggested in #17019 .
This is testing the `-loadblock` option by using the scripts in `contrib/linearize` to generate a `bootstrap.dat` file and starting a disconnected node with it. So it is also testing the linearize scripts which were untested before and needed to be made available for the CI environment, hence they are added to `DIST_CONTRIB` in `Makefile.am`.
ACKs for top commit:
laanwj:
ACK 89339d1460
Tree-SHA512: aede0cd6e8b21194973f3633bc07fa2672d66a6f85dfe6a57cee2bb269a65d19ea49d5f9ed7914a173b3847c76e70257aa865f44bde170c1999d9655b4862d1c
prototypes used in src/test/script_tests.cpp:
- CMutableTransaction BuildCreditingTransaction(const CScript& scriptPubKey, int nValue = 0);
- CMutableTransaction BuildSpendingTransaction(const CScript& scriptSig, const CScriptWitness& scriptWitness, const CTransaction& txCredit);
prototypes used in bench/verify_script.cpp:
- CMutableTransaction BuildCreditingTransaction(const CScript& scriptPubKey);
- CMutableTransaction BuildSpendingTransaction(const CScript& scriptSig, const CMutableTransaction& txCredit);
The more generic versions from the script tests are moved into a new file pair
transaction_utils.cpp/h and the calls are adapted accordingly in the
verify_script benchmark (passing the nValue of 1 explicitely for
BuildCreditingTransaction(), passing empty scriptWitness explicitely and
converting txCredit parameter to CTransaction in BuildSpendingTransaction()).
0616138a07 tests: Remove no longer needed UBSan suppressions (issues fixed). Add documentation. (practicalswift)
Pull request description:
Remove no longer needed UBSan suppressions (issues fixed). Add documentation.
This PR is the CI-only subset of #17208 (which touches code).
From a fuzzing perspective it would be really nice to be able to run UBSan with as few suppressions as possible :)
Top commit has no ACKs.
Tree-SHA512: a926ab3e80e12a805af110fbff470cdc61ef4db536919a5b8896ea8b70f761114a52d9b1c0f48b11c1d48338351bf2e003e01ce60c613612f26ba298dcc29cd9
The serialization/deserialization methods for the classes CExtKey and
CExtPubKey were only used in the BIP32 unit tests, where the relevant parts are
removed as well.
facec1c643 wallet: Avoid showing GUI popups on RPC errors (MarcoFalke)
Pull request description:
RPC errors and warnings are shown as popups in the GUI instead of being returned to the RPC caller. For example,
```
$ ./src/bitcoin-cli loadwallet $(pwd)/./test/functional/data/wallets/high_minversion/
error code: -4
error message:
Wallet loading failed.
```
gives me a GUI popup and no reason why loading the wallet failed.
After this pull request:
```
$ ./src/bitcoin-cli loadwallet $(pwd)/./test/functional/data/wallets/high_minversion/
error code: -4
error message:
Wallet loading failed: Error loading /home/marco/workspace/btc_bitcoin_core/./test/functional/data/wallets/high_minversion/wallet.dat: Wallet requires newer version of Bitcoin Core
ACKs for top commit:
laanwj:
Code review ACK facec1c643
Tree-SHA512: c8274bbb02cfcf71676eeec1e773e51fb3538cf93f82e7cb8536f4716d44ed819cdc162dfc039ac7386a4db381a734cdb27fd32567043a1180c02519fbcba194
57e2edea0b Send amount shows minimum amount placeholder (JeremyCrookshank)
Pull request description:
Noticed that there wasn't a default value for the send amount. However if you put a value in or click the up and down arrows you're unable to get it blank again, so it makes sense that it has a default value. I hope this also makes it more clear that users can send less than 1 BTC if it shows the 8 decimal places
PR:
ACKs for top commit:
promag:
ACK 57e2edea0b.
GChuf:
ACK 57e2edea0b
laanwj:
ACK 57e2edea0b, this is a surprisingly compact solution too
Tree-SHA512: 354590d2a88231b8649f7ae985c8a7864d74ca0e1f8603cb1730ba46747084de90ee6285ce4d39ee04b054fb9cd2d78ebc71146f3af694c37a8a3aff7f051800
fa677d1801 ci: Remove redundant check for TRAVIS_OS_NAME (MarcoFalke)
fadccb263b doc: Document that GNU tools are required for linters (MarcoFalke)
4444704ca9 ci: Cleanup macOS runs (MarcoFalke)
Pull request description:
* Remove a commented out cleanup task in `before_cache`
* Remove the linter run on macOS, and document that GNU tools are required to run the linters
ACKs for top commit:
Sjors:
Code review ACK fa677d1801
laanwj:
ACK fa677d1801
ryanofsky:
Code review ACK fa677d1801 for new third commit replacing TRAVIS_OS_NAME check with NO_DEPENDS setting
Tree-SHA512: 9122a63bbe7887d9e379123152ea4ba44324cb18033b9e6b45bfdb1af665c10ea598564b9fcd57330d208a08e4696e41b4d6175f05f0843a3a76530da114f8c6
e892f9648a random: remove call to RAND_screen() (Windows only) (fanquake)
Pull request description:
Follow up to https://github.com/bitcoin/bitcoin/pull/17151 where there were multiple calls to also remove our call to RAND_screen().
ACKs for top commit:
MarcoFalke:
unsigned ACK e892f9648a
laanwj:
ACK e892f9648a
Tree-SHA512: 1b846016d91e8113f90466b61fcaf0574edb6b4726eba1947549e2ac28907e1318d893f7b303e756f19730c8507c79b10e08d54b97153224b585ff1e0ac1953e
7ca68e1461 wallet: Remove unused GetLabelName (Sebastian Falbesoner)
Pull request description:
While taking a look at #17198 I noticed that the method `CWallet::GetLabelName(...)` is not used anymore, since the `account` API was removed (c9c32e6b84).
ACKs for top commit:
practicalswift:
ACK 7ca68e1461
promag:
ACK 7ca68e1461.
fanquake:
ACK 7ca68e1461
Tree-SHA512: 6825d77a85934e2368a3fb44c8db0ed0872aa5606e3761decb0a6b7e3773277afa7021bf1f71009207c3961cdd0a1c448854ea2fd8be95c3afec466254faf82d
f09ba060ca doc: Added instructions for how to add an upsteam to forked repo (dannmat)
Pull request description:
As a first time git developer, I struggled to understand whether to create a new fork for each pull request or not.
After asking the IRC chat, I have added this to the documentation to further help new developers using git.
ACKs for top commit:
fanquake:
ACK f09ba060ca - For such a simple change, I think we've bike-shed this enough already. The `bitcoin/bitcoin` repo isn't really where anyone should be learning how to use `git` etc, but I think linking out here is ok.
Tree-SHA512: e0e9d655d0725e0128673afedb81dc5ba9387968fcbb681de7e50155a2cfa1a7f39fad040b596f4de9ad6727a1a8a90fd3d36eaa5242bc12186c3b82abd23fb2
As a first time git developer, I struggled to understand whether to create a new fork for each pull request or not.
After asking the IRC chat, I have added this to the documentation to further help new developers using git.
Co-Authored-By: Michael <fanquake@gmail.com>
