0a50019fde Walk pindexBestHeader back to ChainActive().Tip() if it is invalid (Matt Corallo)
Pull request description:
Instead of keeping pindexBestHeader set to the best header we've
ever seen, reset it back to our validated tip if we find an ancestor
of it turns out to be invalid. While the name is now a bit confusing,
this matches much better with how it is used in practice, see below.
Further, this opens up more use-cases for it in the future, namely
aggressively searching for new peers in case we have discovered
(possibly via some covert channel) headers which we do not know to be
invalid, but which we cannot find block data for.
Places pindexBestHeader is used:
* Various GUI displays of the best header and getblockchaininfo["headers"],
I don't think changing this is bad, and if anything this is less confusing
in the presence of an invalid block.
* IsCurrentForFeeEstimation(): If anything I think ensuring pindexBestHeader
isn't some crazy invalid chain is better than the alternative, even in the
case where you are rejecting the current chain due to hardware error (since
hopefully in that case you won't get any new blocks anyway).
* ConnectBlock assumevalid checks: We use pindexBestHeader to check that the
block we're connecting leads to something with nMinimumChainWork (preventing
a user-set assumevalid from having bogus work) and that the block we're
connecting leads to pindexBestHeader (I'm not too worried about this one -
it's nice to "disable" assumevalid if we have a long invalid headers chain,
but I don't see it as a critical protection).
* BlockRequestAllowed() uses pindexBestHeader as its target to ensure the
requested block is within a month of the "current chain". I don't think this
is a meaningful difference, if we're rejecting the current tip we're
trivially fingerprintable anyway, and if the chain really does have a bunch
of invalid crap near the tip, using the best not-invalid header is likely a
better criteria.
* ProcessGetBlockData uses pindexBestHeader as the "current chain" definition
of whether a block request is "historical" for the purpose of bandwidth
limiting. Similarly, I don't see why this is a meaningful change.
* We use pindexBestHeader for requesting missing headers on receipt of a
headers/compact block message or block inv as well as for initial getheaders.
I think this is definitely wrong, using the best not-invalid header for such
requests is much better.
* We use pindexBestHeader to define the "current chain" for deciding when
we're close to done with initial headers sync. I don't think this is a
meaningful change.
* We use pindexBestHeader to decide if initial headers sync has timed out. If
we're rejecting the chain due to hardware error this may result in
additional cases where we ban a peer, but this is already true, so I think
its fine.
ACKs for top commit:
fjahr:
ACK 0a50019fde
kallewoof:
ACK 0a50019fde
ariard:
utACK 0a50019
Tree-SHA512: 2ecfa973a9878a00313ae7ede94a9bd7710e0caf55b544b10bbc46dc463a0478cbaf477e6cdd072356d5a0c5fb3848e9339284af785a2995c20bae8bd23f23e5
bd5a026928 Make UpdateTransactionsFromBlock use Epochs (Jeremy Rubin)
2ccb7cca4a Add Epoch Guards to CTXMemPoolEntry and CTxMemPool (Jeremy Rubin)
Pull request description:
UpdateTransactionsFromBlock is called during a re-org. When a re-org occurs, all of the transactions in the mempool may be descendants from a transaction which is in the pre-reorg block. This can cause us to propagate updates, worst case, to every transaction in the mempool.
Because we construct a `setEntries setChildren`, which is backed by a `std::set`, it is possible that this algorithm is `O(N log N)`.
By using an Epoch visitor pattern, we can limit this to `O(N)` worst case behavior.
Epochs are also less resource intensive than almost any set option (e.g., hash set) because they are allocation free.
This PR is related to https://github.com/bitcoin/bitcoin/pull/17268, it is a small subset of the changes which have been refactored slightly to ease review. If this PR gets review & merge, I will follow up with more PRs (similar to #17268) to improve the mempool
ACKs for top commit:
sdaftuar:
ACK bd5a026928
adamjonas:
Just to summarize for those looking to review - as of bd5a026 there are 3 ACKs (@sdaftuar, @ariard, and @hebasto) and one "looks good" from @ajtowns with no NACKs or any show-stopping concerns raised.
ajtowns:
ACK bd5a026928 (code review)
ariard:
Code review ACK bd5a026
hebasto:
ACK bd5a026928, modulo some nits and a typo.
Tree-SHA512: f0d2291085019ffb4e1119edeb9f4a89c1a572d1cb5b4bdf5743dd0152e721e1935f5155dcae84e1e5bda5ffdf6224c488c1e200bd33bedca9f5ca22d5f5139f
9a299a59cc net: reference instead of copy in BlockConnected range loop (Jon Atack)
Pull request description:
Reference elements in range for loop instead of copying them and
fix Clang `-Wrange-loop-analysis` warning introduced in a029e18
```
net_processing.cpp:1185:25: warning: loop variable 'ptx' of
type 'const std::shared_ptr<const CTransaction>' creates a copy from
type 'const std::shared_ptr<const CTransaction>' [-Wrange-loop-analysis]
for (const auto ptx : pblock->vtx) {
^
net_processing.cpp:1185:14: note: use reference type
'const std::shared_ptr<const CTransaction> &' to prevent copying
for (const auto ptx : pblock->vtx) {
^~~~~~~~~~~~~~~~
1 warning generated.
```
ACKs for top commit:
Empact:
ACK 9a299a59cc
MarcoFalke:
ACK 9a299a59cc
promag:
ACK 9a299a59cc.
elichai:
ACK 9a299a59cc
emilengler:
ACK 9a299a5.
Tree-SHA512: 9284d1b00684877505454a05071212758c8cea083534e2eec09bfc8a9c3059eea811d2008f6a5a678539444f0d5b3134db1bd23da6514b3d3a1440634c8b53be
d3bc184081 doc: update release notes with getaddressinfo label deprecation (Jon Atack)
72af93f364 test: getaddressinfo label deprecation test (Jon Atack)
d48875fa20 rpc: deprecate getaddressinfo label field (Jon Atack)
dc0cabeda4 test: remove getaddressinfo label tests (Jon Atack)
c7654af6f8 doc: address pr17578 review feedback (Jon Atack)
Pull request description:
This PR builds on #17578 (now merged) and deprecates the rpc getaddressinfo `label` field. The deprecated behavior can be re-enabled by starting bitcoind with `-deprecatedrpc=label`.
See http://www.erisian.com.au/bitcoin-core-dev/log-2019-11-22.html#l-622 and https://github.com/bitcoin/bitcoin/pull/17283#issuecomment-554458001 for more context.
Reviewers: This PR may be tested manually by building, then running bitcoind with and without the `-deprecatedrpc=label` flag while verifying the rpc getaddressinfo output and help text.
Next step: add support for multiple labels.
ACKs for top commit:
jnewbery:
ACK d3bc184081
laanwj:
ACK d3bc184081
meshcollider:
utACK d3bc184081
Tree-SHA512: f954402884ec54977def332c8160fd892f289b0d2aee1e91fed9ac3220f7e5b1f7fc6421b84cc7a5c824a0582eca4e6fc194e4e33ddd378c733c8941ac45f56d
cb8a86d9f9 gui: Remove WalletView and BitcoinGUI circular dependency (João Barbosa)
ac3d10777d gui: Add transactionClicked and coinsSent signals to WalletView (João Barbosa)
Pull request description:
Essentially moves the code in `WalletView::setBitcoinGUI` to the only caller. Two new signals are added beforehand in the first commit so that the connections in `WalletFrame` are all from the wallet view.
ACKs for top commit:
hebasto:
ACK cb8a86d9f9, tested on Linux Mint 19.3.
jonasschnelli:
utACK cb8a86d9f9
Tree-SHA512: 250316cd3689e51c8cded9ccd75963c836dcafa6db25d684f2aa691dea9738895f9140793e0f925784909e39f8257f7e1c7d611e8bd6d6634e1a50333f4ddb1e
3aee10b80b gui: Drop ShutdownWindow dependency to BitcoinGUI (João Barbosa)
61eb058cc1 gui: Drop BanTableModel dependency to ClientModel (João Barbosa)
Pull request description:
`ShutdownWindow::showShutdownWindow` just needs a widget to center the shutdown window and to borrow its title.
ACKs for top commit:
hebasto:
ACK 3aee10b80b, since previous review only suggested change `QWidget` --> `QMainWindow`
jonasschnelli:
utACK 3aee10b80b
Tree-SHA512: e15cb6ee274730bd071d3d97b540c5059e5c655248d69a37c3fd00f2aacc6cfcb36b9a65755718027e15482ec8e5e85534c1dc13d0ddb4e0680df03fbf6571f2
a029e18c2b Use rolling bloom filter of recent block tx's for AlreadyHave() check (Suhas Daftuar)
Pull request description:
In order to determine whether to download or process a relayed transaction, we first try to check whether we already have the transaction -- either in the mempool, in our filter of recently rejected transactions, in our orphan pool, or already confirmed in a block.
Prior to this commit, the heuristic for checking whether a transaction was confirmed in a block is based on whether there's a coin cache entry corresponding to the 0- or 1-index vout of the tx. While that is a quick check, it is very imprecise (eg if those outputs were already spent in another block, we wouldn't detect that the transaction has already been confirmed) -- we can do better by just keeping a rolling bloom filter of the transactions in recent blocks, which will better capture the case of a transaction which has been confirmed and then fully spent.
This should reduce the bandwidth that we waste by requesting transactions which will not be accepted to the mempool.
To avoid relay problems for transactions which have been included in a recent block but then reorged out of the chain, we clear the bloom filter whenever a block is disconnected.
ACKs for top commit:
MarcoFalke:
re-ACK a029e18c2b only stylistic and comment fixups 🍴
sipa:
utACK a029e18c2b
jonatack:
Code review ACK a029e18c2b also built/ran tests and am running bitcoind with mempool debug logging and custom logging. Looked a bit into CRollingBloomFilter and also the mempool median time past checks mentioned above; I don't have a deep understanding of those areas yet but the concept here and changes LGTM. Tests and other optimisations could be added as a follow-up. In favor of seeing this move forward if no major immediate concerns.
Tree-SHA512: 784c9a35bcd3af5db469063ac7d26b4bac430e451e5637a34d8a538c3ffd1433abdd3f06e5584e7a84bfa9e791449e61819397b5a6c7890fa59d78ec3ba507b2
c7ec9a1888 Add missing supported rpcs to doc/descriptors.md (Andrew Toth)
Pull request description:
Improve descriptor docs by adding missing rpcs.
ACKs for top commit:
fanquake:
ACK c7ec9a1888 - I think this has been bikeshed enough.
jonatack:
ACK c7ec9a1888
Tree-SHA512: 783219928ed7edc904b507bb30e2eefd8ca9f11225e1460fedecd755f9511055adcc52cc49f66ba840e121883e40753061db76a243ee6e0091daf1fc396ae59a
085423b978 Remove GitHub Actions CI workflow. (Aaron Clauson)
Pull request description:
While the GitHub Action CI workflow has permissions to make commits it's not suitable.
As per #17803.
ACKs for top commit:
fanquake:
ACK 085423b978
Tree-SHA512: 62c19dda6164a563dc87e394314afc57ef2494b7e4af148b83220f1b2b0bef9ff309b5cde5d9bed3633429b77041871993ac5c73b0cbe7f720d6a4e60e3e816a
b951b0973c on startup, write config options to debug.log (Larry Ruane)
Pull request description:
When a developer is examining `debug.log` after something goes wrong, it's often useful to know the exact options the failing instance of `bitcoind` was started with. Sometimes the `debug.log` file is all that's available for the analysis. This PR logs the `bitcoin.conf` entries and command-line arguments to `debug.log` on startup.
ACKs for top commit:
MarcoFalke:
ACK b951b0973c🐪
jonatack:
ACK b951b0973c reviewed diff, re-code review, built, ran tests, launched bitcoind and reviewed debug log output, verified value of `str` debug log in the added unit test.
Tree-SHA512: bbca4fb3d49f99261758302bde0b8b67300ccc72e7380b01f1f66a146ae8a008a045df0ca5ca9664caff034d0ee38ea7ef38a50f38374525608c07ba52790358
aaaae4d0eb test: Add p2p test for forcerelay permission (MarcoFalke)
fa6b57bcaa test: Fix whitespace in p2p_permissions.py (MarcoFalke)
faf40810d7 test: Make msg_tx a witness tx (MarcoFalke)
Pull request description:
The commit `test: Make msg_tx a witness tx` is needed so that the python mininode does not strip the witness from transactions before sending them over p2p. The commit should also be done to keep symmetry with msg_block. See:
* tests: Make msg_block a witness block #15982
ACKs for top commit:
laanwj:
ACK aaaae4d0eb
Tree-SHA512: b4b546c88f7f0576cb512f0872bc6bef9d4df65783803f226986e56175937f418aa1ed906417ac909f27f1fd521d64629621fda83250fa925c46ef9513db0e4c
cc668d06fb tests: Add fuzzing harness for strprintf(...) (practicalswift)
ccc3c76e2b tests: Add fuzzer strprintf to FUZZERS_MISSING_CORPORA (temporarily) (practicalswift)
6ef04912af tests: Update FuzzedDataProvider.h from upstream (LLVM) (practicalswift)
Pull request description:
Add fuzzing harness for `strprintf(…)`.
Update `FuzzedDataProvider.h`.
Avoid hitting some issues in tinyformat (reported upstreams in https://github.com/c42f/tinyformat/issues/70).
---
Found issues in tinyformat:
**Issue 1.** The following causes a signed integer overflow followed by an allocation of 9 GB of RAM (or an OOM in memory constrained environments):
```
strprintf("%.777777700000000$", 1.0);
```
**Issue 2.** The following causes a stack overflow:
```
strprintf("%987654321000000:", 1);
```
**Issue 3.** The following causes a stack overflow:
```
strprintf("%1$*1$*", -11111111);
```
**Issue 4.** The following causes a `NULL` pointer dereference:
```
strprintf("%.1s", (char *)nullptr);
```
**Issue 5.** The following causes a float cast overflow:
```
strprintf("%c", -1000.0);
```
**Issue 6.** The following causes a float cast overflow followed by an invalid integer negation:
```
strprintf("%*", std::numeric_limits<double>::lowest());
```
Top commit has no ACKs.
Tree-SHA512: 9b765559281470f4983eb5aeca94bab1b15ec9837c0ee01a20f4348e9335e4ee4e4fecbd7a1a5a8ac96aabe0f9eeb597b8fc9a2c8faf1bab386e8225d5cdbc18
1b96a3cd1e tests: reset fIsBareMultisigStd after bare-multisig tests (fanquake)
Pull request description:
Fixes: #18015
The bug this fixes is two-part.
1. The `fIsBareMultisigStd` global is being reused by other tests,
such as [script_p2sh_tests(set)](https://github.com/bitcoin/bitcoin/blob/master/src/test/script_p2sh_tests.cpp#L150), after being set to false.
2. The order our tests run in doesn't always? seem to be random,
which meant that the `script_p2sh` tests would only fail if they
were run in an order where the `transaction_tests` ran first,
mutating the `fIsBareMultisigStd` global.
This doesn't seem to happen when running make check, but if you
run `src/test/test_bitcoin and pass --random=99999`, the failure
in `script_p2sh` will occur (on most, but maybe not all systems):
```bash
src/test/test_bitcoin --random=99999
Running 389 test cases...
test/script_p2sh_tests.cpp:200: error: in "script_p2sh_tests/set": txTo[1].IsStandard
test/script_p2sh_tests.cpp:200: error: in "script_p2sh_tests/set": txTo[2].IsStandard
test/script_p2sh_tests.cpp:200: error: in "script_p2sh_tests/set": txTo[3].IsStandard
*** 3 failures are detected in the test module "Bitcoin Core Test Suite"
```
The new test for bare multisig was introduced in #17502.
ACKs for top commit:
Empact:
Code Review ACK 1b96a3cd1e
theStack:
ACK https://github.com/bitcoin/bitcoin/pull/18018/commits/1b96a3cd1ebe725896f59614903184289fe62c
Tree-SHA512: fd7578f9f3faa44d236cd007fc25e31f061acabdb8458559fde0e67d11ab5cafed15305993270c9943a50326574bc5f5301b09494a5b0d2de69e64978093ed45
1115ba693b psbt_wallet_tests: use unique_ptr for GetSigningProvider (Anthony Towns)
Pull request description:
#17261 changed GetSigningProvider to return a unique_ptr, but #17156 made psbt_wallet_tests use it as well, and wasn't correspondingly updated.
ACKs for top commit:
fanquake:
ACK 1115ba693b
meshcollider:
Thanks! utACK 1115ba693b
Tree-SHA512: f0191c9b00780e6d1445fa4ec531456758b468b5bca8660474d22b1edb5f48a636a940656c9bdbe466b8bffad7af1e57e0756239906e901d60c69c3124d3bff4
3f373659d7 Refactor: Replace SigningProvider pointers with unique_ptrs (Andrew Chow)
3afe53c403 Cleanup: Drop unused GUI learnRelatedScripts method (Andrew Chow)
e2f02aa59e Refactor: Copy CWallet signals and print function to LegacyScriptPubKeyMan (Andrew Chow)
c729afd0a3 Box the wallet: Add multiple keyman maps and loops (Andrew Chow)
4977c30d59 refactor: define a UINT256_ONE global constant (Andrew Chow)
415afcccd3 HD Split: Avoid redundant upgrades (Andrew Chow)
01b4511206 Make UpgradeKeyMetadata work only on LegacyScriptPubKeyMan (Andrew Chow)
4a7e43e846 Store p2sh scripts in AddAndGetDestinationForScript (Andrew Chow)
501acb5538 Always try to sign for all pubkeys in multisig (Andrew Chow)
81610eddbc List output types in an array in order to be iterated over (Andrew Chow)
eb81fc3ee5 Refactor: Allow LegacyScriptPubKeyMan to be null (Andrew Chow)
fadc08ad94 Locking: Lock cs_KeyStore instead of cs_wallet in legacy keyman (Andrew Chow)
f5be479694 wallet: Improve CWallet:MarkDestinationsDirty (João Barbosa)
Pull request description:
Continuation of wallet boxes project.
Actually makes ScriptPubKeyMan an interface which LegacyScriptPubkeyMan. Moves around functions and things from CWallet into LegacyScriptPubKeyMan so that they are actually separate things without circular dependencies.
***
Introducing the `ScriptPubKeyMan` (short for ScriptPubKeyManager) for managing scriptPubKeys and their associated scripts and keys. This functionality is moved over from `CWallet`. Instead, `CWallet` will have a pointer to a `ScriptPubKeyMan` for every possible address type, internal and external. It will fetch the correct `ScriptPubKeyMan` as necessary. When fetching new addresses, it chooses the `ScriptPubKeyMan` based on address type and whether it is change. For signing, it takes the script and asks each `ScriptPubKeyMan` for whether that `ScriptPubKeyMan` considers that script `IsMine`, whether it has that script, or whether it is able to produce a signature for it. If so, the `ScriptPubKeyMan` will provide a `SigningProvider` to the caller which will use that in order to sign.
There is currently one `ScriptPubKeyMan` - the `LegacyScriptPubKeyMan`. Each `CWallet` will have only one `LegacyScriptPubKeyMan` with the pointers for all of the address types and change pointing to this `LegacyScriptPubKeyMan`. It is created when the wallet is loaded and all keys and metadata are loaded into it instead of `CWallet`. The `LegacyScriptPubKeyMan` is primarily made up of all of the key and script management that used to be in `CWallet`. For convenience, `CWallet` has a `GetLegacyScriptPubKeyMan` which will return the `LegacyScriptPubKeyMan` or a `nullptr` if it does not have one (not yet implemented, but callers will check for the `nullptr`). For purposes of signing, `LegacyScriptPubKeyMan`'s `GetSigningProvider` will return itself rather than a separate `SigningProvider`. This will be different for future `ScriptPubKeyMan`s.
The `LegacyScriptPubKeyMan` will also handle the importing and exporting of keys and scripts instead of `CWallet`. As such, a number of RPCs have been limited to work only if a `LegacyScriptPubKeyMan` can be retrieved from the wallet. These RPCs are `sethdseed`, `addmultisigaddress`, `importaddress`, `importprivkey`, `importpubkey`, `importmulti`, `dumpprivkey`, and `dumpwallet`. Other RPCs which relied on the wallet for scripts and keys have been modified in order to take the `SigningProvider` retrieved from the `ScriptPubKeyMan` for a given script.
Overall, these changes should not effect how everything actually works and the user should experience no difference between having this change and not having it. As such, no functional tests were changed, and the only unit tests changed were those that were directly accessing `CWallet` functions that have been removed.
This PR is the last step in the [Wallet Structure Changes](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Class-Structure-Changes).
ACKs for top commit:
instagibbs:
re-utACK 3f373659d7
Sjors:
re-utACK 3f373659d7 (it still compiles on macOS after https://github.com/bitcoin/bitcoin/pull/17261#discussion_r370377070)
meshcollider:
Tested re-ACK 3f373659d7
Tree-SHA512: f8e2b8d9efa750b617691e8702d217ec4c33569ec2554a060141d9eb9b9a3a5323e4216938e2485c44625d7a6e0925d40dea1362b3af9857cf08860c2f344716
The bug this fixes is two-part.
1.The fIsBareMultisigStd global is being reused by other tests,
i.e script_p2sh_tests(set), after being set to false.
2. The order our tests run in doesn't always? seem to be random,
which meant that the script_p2sh tests would only fail if they
were run in an order where transaction_tests ran first, mutating
the fIsBareMultisigStd global.
This doesn't seem to happen when running make check, but if you
run src/test/test_bitcoin and pass --random=99999, the failure
in script_p2sh:
test/script_p2sh_tests.cpp:200: error: in "script_p2sh_tests/set": txTo[1].IsStandard
will occur (on most systems).
The new test was introduced in 1bb5d517aa.
fa1a46e7f4 build: Fix appveyor test_bitcoin build of *.raw (MarcoFalke)
Pull request description:
Fixes#18020
Top commit has no ACKs.
Tree-SHA512: c0b3ca4f95b46543bb3bc6d254300c832a69feca79f5de4e13cafd4c962ae53903069ec7a8c9573761eefa5cec617992b70750b067ee42231dc74170ba6c3b10
f41d589669 Document better -keypool as a look-ahead safety mechanism (Antoine Riard)
Pull request description:
If after a backup, an address is issued beyond the initial
keypool range and none of the addresses in this range
is seen onchain, if a wallet is restored from backup, even in
case of rescan, funds may be loss due to the look-ahead
buffer not being incremented and so restored wallet not detecting
onchain out-of-range address as derived from its seed.
This scenario is theoretically unavoidable due to the requirement
of the keypool to have a max size. However, given the default
keypool size, this is unlikely. Document better keypool size
implications to avoid user setting a too low value.
While reviewing #17681, it took me a while to figure out the safety implications of keypool, I find it would be better to document this a bit farther to avoid users shooting themselves in the foot. For further context & discussion, see https://github.com/bitcoin/bitcoin/pull/17681#issuecomment-563613452
ACKs for top commit:
ryanofsky:
Code review ACK f41d589669. Just "Warning:" prefix added since the last review
jonatack:
ACK f41d589669 code review and build/test. The added `Warning:` since last review is a good addition.
Tree-SHA512: d3d0ee88fcdfc5c8841a2bd4bada0e4eeb412a0dce5054e5fb023643c2fa57206a0f3efb06890c245528dc4431413ed2fd5645b9319d26245d044c490b7f0db0
In order to determine whether to download or process a relayed transaction, we
try to determine if we already have the transaction, either in the mempool, in
our recently rejected filter, in our orphan pool, or already confirmed in the
chain itself.
Prior to this commit, the heuristic for checking the chain is based on whether
there's an output corresponding to the 0- or 1-index vout in our coin cache.
While that is a quick check, it is very imprecise (say if those outputs were
already spent in a block) -- we can do better by just keeping a rolling bloom
filter of the transactions in recent blocks, which will capture the case of a
transaction which has been confirmed and then fully spent already.
To avoid relay problems for transactions which have been included in a recent
block but then reorged out of the chain, we clear the bloom filter whenever a
block is disconnected.
4de934b9b5 Convert compression.h to new serialization framework (Pieter Wuille)
ca34c5cba5 Add FORMATTER_METHODS, similar to SERIALIZE_METHODS, but for formatters (Pieter Wuille)
Pull request description:
This is the next piece of the puzzle from #10785. It includes:
* The `FORMATTER_METHODS` macro, similar to `SERIALIZE_METHODS`, for defining a formatter with a unified serialization/deserialization implementation.
* Updating `compression.h` to consist of 3 formatters, rather than old-style wrappers (`ScriptCompression`, `AmountCompression`, `TxOutCompression`).
ACKs for top commit:
laanwj:
code review ACK 4de934b9b5
ryanofsky:
Code review ACK 4de934b9b5. Only change since last review is removing REF usages
Tree-SHA512: d52ca21eb1ce87d9bc3c90d00c905bd4fada522759aaa144c02a58b4d738d5e8647c0558b8ce393c707f6e3c4d20bf93781a2dcc1e1dcbd276d9b5ffd0e02cd6
b6c3e84e87 doc: Improve fuzzing docs for macOS users (Fabian Jahr)
Pull request description:
Adds several helpful hints for macOS users trying to get fuzzers to run locally using AFL or libFuzzer. These are partly based on this comment https://github.com/bitcoin/bitcoin/issues/17657#issuecomment-562869600 and discussions in the review club for #17860. See: https://bitcoincore.reviews/17860.html
Based on the doc in the current state I could not compile fuzzers for AFL or libFuzzer. Using these hints, I can
- compile and run fuzzers with AFL
- compile but **not** run fuzzers with libFuzzer
Fuzzers compiled with libFuzzers may be running but don't produce any output. Looking for others to test this to see if it is an issue with my local system. Especially interesting if you have been running libFuzzer fuzzers successfully on macOS before.
Edit: Closes#17914
ACKs for top commit:
MarcoFalke:
ACK b6c3e84e87
Sjors:
ACK b6c3e84
fanquake:
ACK b6c3e84e87 - I think this has been nitpicked enough, and importantly the commands look better now.
Tree-SHA512: fdbacbcf10e9353a4ac3d22edf88663e33185ad2f244b986ff74c513de05f9fa62c4d8b17985d2f9288834c124b352cf52280627b5ff095735b411b12482e2ec
3c1bc40205 Add extra logging of asmap use and bucketing (Gleb Naumenko)
e4658aa8ea Return mapped AS in RPC call getpeerinfo (Gleb Naumenko)
ec45646de9 Integrate ASN bucketing in Addrman and add tests (Gleb Naumenko)
8feb4e4b66 Add asmap utility which queries a mapping (Gleb Naumenko)
Pull request description:
This PR attempts to solve the problem explained in #16599.
A particular attack which encouraged us to work on this issue is explained here [[Erebus Attack against Bitcoin Peer-to-Peer Network](https://erebus-attack.comp.nus.edu.sg/)] (by @muoitranduc)
Instead of relying on /16 prefix to diversify the connections every node creates, we would instead rely on the (ip -> ASN) mapping, if this mapping is provided.
A .map file can be created by every user independently based on a router dump, or provided along with the Bitcoin release. Currently we use the python scripts written by @sipa to create a .map file, which is no larger than 2MB (awesome!).
Here I suggest adding a field to peers.dat which would represent a hash of asmap file used while serializing addrman (or 0 for /16 prefix legacy approach).
In this case, every time the file is updated (or grouping method changed), all buckets will be re-computed.
I believe that alternative selective re-bucketing for only updated ranges would require substantial changes.
TODO:
- ~~more unit tests~~
- ~~find a way to test the code without including >1 MB mapping file in the repo.~~
- find a way to check that mapping file is not corrupted (checksum?)
- comments and separate tests for asmap.cpp
- make python code for .map generation public
- figure out asmap distribution (?)
~Interesting corner case: I’m using std::hash to compute a fingerprint of asmap, and std::hash returns size_t. I guess if a user updates the OS to 64-bit, then the hash of asap will change? Does it even matter?~
ACKs for top commit:
laanwj:
re-ACK 3c1bc40205
jamesob:
ACK 3c1bc40205 ([`jamesob/ackr/16702.3.naumenkogs.p2p_supplying_and_using`](https://github.com/jamesob/bitcoin/tree/ackr/16702.3.naumenkogs.p2p_supplying_and_using))
jonatack:
ACK 3c1bc40205
Tree-SHA512: e2dc6171188d5cdc2ab2c022fa49ed73a14a0acb8ae4c5ffa970172a0365942a249ad3d57e5fb134bc156a3492662c983f74bd21e78d316629dcadf71576800c
b35567fe0b test: only declare a main() when fuzzing with AFL (fanquake)
Pull request description:
This fixes fuzzing using [libFuzzer](https://llvm.org/docs/LibFuzzer.html) on macOS, which caused a few issues during the recent review club. macOS users could only fuzz using afl, or inside a VM.
It seems that the `__attribute__((weak))` marking is not quite enough to properly mark `main()` as weak on macOS. See Apples docs on [Frameworks and Weak Linking](https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPFrameworks/Concepts/WeakLinking.html#//apple_ref/doc/uid/20002378-107262-CJBJAEID).
Have tested fuzzing using libFuzzer and AFL with this patch.
ACKs for top commit:
MarcoFalke:
ACK b35567fe0b
fjahr:
ACK b35567f
Tree-SHA512: b881fdd98c7e1587fcf44debd31f5e7a52df938059ab91c41d0785077b3329b793e051a2bf2eee64488b9f6029d9288c911052ec23ab3ab8c0561a2be1682dae
e80317be5f refactor: Remove redundant conditional (Bushstar)
Pull request description:
Conditional check against fMaster is now redundant as it is already checked as true. This originally made sense as the outer conditional was:
f9cae832e6/src/checkqueue.h (L86)
Removal of fQuit happened in the commit below.
30ded3e3d8 (diff-88316c9aa9514c038c9304297e672da5)
ACKs for top commit:
theStack:
ACK e80317be5f
hebasto:
ACK e80317be5f, I have reviewed the code, and it looks OK, I agree it can be merged.
promag:
ACK e80317be5f.
emilengler:
re-ACK e80317be5f
practicalswift:
ACK e80317be5f
Empact:
ACK e80317be5f
Tree-SHA512: 136ea1d02e3d65100a8758730617ccede7864e08e8404e42e65d45d4bf95a3bfea2ab9895c6e8833abd654557d3efbba02b25297a2a5eefc36a11e97bbe9134f
deaa6dd144 psbt: check output index is within bounds before accessing (Andrew Chow)
f1ef7f0aa4 Don't calculate tx fees for PSBTs with invalid money values (Andrew Chow)
Pull request description:
Fixes#17149
Two classes of issues were found by the psbt fuzzer: values out of range and causing overflows, and prevout indexes being out of range. This PR fixes both.
When accessing a specific output using the index given in the tx, check that it is actually a possible output before trying to access the output.
When summing and checking amounts for `decodepsbt` and `analyzepsbt`, make sure that the values are actually valid money values.. Otherwise, stop summing and don't show the fee. For `analyzepsbt`, return that the next role is the Creator since the Creator needs to remake the transaction to be valid.
ACKs for top commit:
practicalswift:
ACK deaa6dd144 -- only change since last ACK was the addition of tests
gwillen:
tested ACK deaa6dd, would also like to see this merged!
Tree-SHA512: 06c36720bbb5a7ab1c29f7d15878bf9f0d3e5760c06bff479d412e1bf07bb3e0e9ab6cca820a4bfedaab71bfd7af813807e87cbcdf0af25cc3f66a53a06dbcfd
b3c4d9bac6 test: rename test suite name "tx_validationcache_tests" to match filename (Sebastian Falbesoner)
Pull request description:
Quoting `src/test/README.md`, '`Adding test cases`':
> "The file naming convention is `<source_filename>_tests.cpp`
> and such files should wrap their tests in a test suite
> called `<source_filename>_tests`."
Currently the unit test source file `txvalidationcache_tests.cpp` contains a unit test suite with the name `tx_validationcache_tests`, which is fixed by this PR. The following shell script shows that this is the only mismatch and for all other unit test source files the test suite names are correct:
```
#!/bin/bash
shopt -s globstar
for test_full_filename in **/*_tests.cpp; do
test_name_file=`basename $test_full_filename .cpp`
test_name_suite=`sed -n "s/^.*TEST_SUITE(\(.*_tests\).*$/\1/p" $test_full_filename`
if [ $test_name_file != $test_name_suite ]; then
echo "TestFilename: $test_name_file != TestSuitname: $test_name_suite"
fi
done
```
ACKs for top commit:
practicalswift:
ACK b3c4d9bac6 -- expected naming is better than unexpected naming :)
kristapsk:
ACK b3c4d9bac6
Tree-SHA512: 29d409b1eb22057ee2cc407508e2580d2bc03f412401df11b8ecf77be5ada6bda8f7d2cb5338c5e079490fa12242c1fd6230a09e47252c1b0d9fe535a828ca4c
88c83636d5 guix: Update documentation for time-machine (Carl Dong)
e6050884fd guix: Pin Guix using `guix time-machine` (Carl Dong)
Pull request description:
An alternative to #16519, pinning our version of Guix and eliminating a `guix pull` and changing the default Guix profile of builders.
I think this method might be superior, as it:
- Eliminates the possibility of future changes to the `guix environment` command line interface breaking our builds
- Eliminates the need to set up a separate channel repo
It is a more general pinning solution than #16519.
-----
The reason why I didn't originally propose this is because `guix time-machine` is a recent addition to Guix, only available since `f675f8dec73d02e319e607559ed2316c299ae8c7`
ACKs for top commit:
fanquake:
ACK 88c83636d5
Tree-SHA512: 85e03b0987ffa86da73e02801e1cd8b7622698d70c4ba4e60561611be1e9717d661c2811a59b3e137b1b8eef2d0ba37c313867d035ebc89c3bd06a23a078064a
Quoting src/test/README.md, 'Adding test cases':
"The file naming convention is `<source_filename>_tests.cpp`
and such files should wrap their tests in a test suite
called `<source_filename>_tests`."
Currently the unit test source file txvalidationcache_tests.cpp contains a unit
test suite with the name tx_validationcache_tests, which is fixed by this commit.
The following shell script shows that this is the only mismatch and for all other
unit test source files the test suite names are correct:
#!/bin/bash
shopt -s globstar
for test_full_filename in **/*_tests.cpp; do
test_name_file=`basename $test_full_filename .cpp`
test_name_suite=`sed -n "s/^.*TEST_SUITE(\(.*_tests\).*$/\1/p" $test_full_filename`
if [ $test_name_file != $test_name_suite ]; then
echo "TestFilename: $test_name_file != TestSuitname: $test_name_suite"
fi
done
