Unfortunately, we need -Wno-unused-command-line-argument as configure will fail to run
with -Werror due to Dash's current build system utilizing -static-libstdc++ indiscriminately,
resulting in Clang warnings of "unused arguments".
* llmq only logging
* llmq only logging
* style: reference on the left
* refactoring
* fix
* style: fix colon location in for loop
Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>
facb71576cd4d2e90fd03e09d29b42fa3d730e8c net: Remove forcerelay of rejected txs (MarcoFalke)
Pull request description:
This removes the code that supposedly handled the forced relay of txs from a permissioned peer that were rejected from our mempool. The removal should be fine, because it is dead code for the following reasons:
* While `RelayTransaction` enqueues the inv for all peers, the inv is never processed because it can not be found in the mempool. See 4a07233076/src/net_processing.cpp (L3862-L3866)
* Even if the peers we intended to send the inv to can somehow reply with a getdata to the never-received inv, they won't receive the tx as a reply because it was never added to the "relay memory" (`mapRelay`)
The dead code is (obviously) untested: https://marcofalke.github.io/btc_cov/total.coverage/src/net_processing.cpp.gcov.html#2574
This feature was (intentionally or accidentally) removed in 4d8993b346, which was released in Bitcoin Core 0.13.0. So all currently supported versions of Bitcoin Core ship without this feature. I am not aware of any complaints about this feature or actual documented use-cases. So instead of reviving an unneeded feature, just remove the dead code.
ACKs for top commit:
hebasto:
ACK facb71576cd4d2e90fd03e09d29b42fa3d730e8c, locally running the unit and functional tests.
Tree-SHA512: bfceae6f2983c1510fa0649a9a63c343cbbc1c4ab3a3698039cccf454c81e58c8f5114b147ed42a1bc867da74c43a5b53764ab14f942e191b6f59079044108b5
aaaae4d0ebd5ef34d81997a73ab9839ba7b4b9e4 test: Add p2p test for forcerelay permission (MarcoFalke)
fa6b57bcaaf4dc65d78316353033b03d171a3beb test: Fix whitespace in p2p_permissions.py (MarcoFalke)
faf40810d7b7f42f3588bfa8a663095aa24001b1 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 aaaae4d0ebd5ef34d81997a73ab9839ba7b4b9e4
Tree-SHA512: b4b546c88f7f0576cb512f0872bc6bef9d4df65783803f226986e56175937f418aa1ed906417ac909f27f1fd521d64629621fda83250fa925c46ef9513db0e4c
5ffaf883b93fb8d3d841c36e808b90d61d39d492 test: eliminiated magic numbers in feature_csv_activation.py (Sebastian Falbesoner)
09f706ab8e47ddfdfa41418f2e7cb6d87661147a test: check for OP_CSV empty stack fail reject reason in feature_csv_activation.py (Sebastian Falbesoner)
cbd345a75c2be26e17fce4c65c0c1ca19a3eb9e0 test: test OP_CSV empty stack fail in feature_csv_activation.py (Sebastian Falbesoner)
Pull request description:
Adds an empty stack failure check for OP_CSV (BIP112) to the functional test `feature_csv_activation.py` by prepending a valid scriptSig with `OP_CHECKSEQUENCEVERIFY`.
If BIP112 is inactive, the operator just behaves as a NOP (for both tx versions 1 and 2) and the transaction remains valid -- if it is active, the tx is invalid due to an empty stack (for both tx versions 1 and 2, as well).
Top commit has no ACKs.
Tree-SHA512: 81102aaead5be11e02b894867fa9a9cc17358ec0eb2f21ce2d3db845b87691d305e6ed7c525f9c7e5bcb3c5c609eb28deca0fbaa3d5e9ff928cecd3b91ff129a
fa45d606461dbf5bf1017d6ab15e89c1bcf821a6 test: Reduce unneeded whitelist permissions in tests (MarcoFalke)
Pull request description:
It makes the tests confusing and fragile when overwriting default command line values that are not needed to be overwritten.
ACKs for top commit:
fanquake:
ACK fa45d606461dbf5bf1017d6ab15e89c1bcf821a6
laanwj:
ACK fa45d606461dbf5bf1017d6ab15e89c1bcf821a6
Tree-SHA512: 8ae5ad8c6be156b1a983adccbca8d868ef841e00605ea88e24227f1b7493987c50b3e62e68dd7dc785ad73d6e14279eb13d7a151cb0a976426fe2fd63ce5cbcd
b902bd66b0f35c5016dc5d7aaf501940935edd62 test: check custom descendant limit in mempool_packages.py (Sebastian Falbesoner)
Pull request description:
This is a follow-up PR to #17435, testing the custom descendant limit, passed by the argument `-limitdescendantcount`. ~~It was more tricky than expected, mainly because we don't know for sure at which point node1 has got all the transactions broadcasted from node0 (for the ancestor test this wasn't a problem since the txs were immediately available through `invalidateblock`) -- a simple `sync_mempools()` doesn't work here since the mempool contents are not equal due to different ancestor/descendant limits. Hence I came up with a "hacky manual sync":~~
1. ~~wait until the mempool has the _expected_ tx count (see conditions below)~~
2. ~~after that, wait some time and get sure that the mempool contents haven't changed in-between~~
~~Like for~~ Similar to the ancestor test, we overall check for ~~three~~ four conditions:
- the # of txs in the node1 mempool is equal to the descendant limit (plus 1 for the parent tx, plus the # txs from the previous ancestor test which are still in) ~~(done by the hacky sync above)~~
- all txs in node1 mempool are a subset of txs in node0 mempool
- part of the constructed descendant-chain (the first ones up to the limit) are contained in node1 mempool
- the remaining part of the constructed descendant-chain (all after the first ones up to the limit) is *not* contained in node1 mempool
ACKs for top commit:
JeremyRubin:
Excellent. utACK b902bd6
Tree-SHA512: 7de96dd248f16ab740e178ac5b64b57ead18cdcf74adfe989709d215e4a67b6b6d20de22c48e885d5f2edc55caaddd44a4261e996c5c87687ceb6a47f1d1fdaf
0653939ac130eddffe40c53ac418bea305d3bf82 Add static_asserts to ser_X_to_Y() methods (Samer Afach)
be94096dfb0c4862e2314cbae4120d7360b08ef2 Fix a violation of C++ standard rules that unions cannot be switched. (Samer Afach)
Pull request description:
Type punning in C++ is not like C. As per the C++ standard, one cannot use unions to convert the bit type. A discussion about this can be found [here](https://stackoverflow.com/questions/25664848/unions-and-type-punning). In C++, a union is supposed to only hold one type at a time. It's intended to be used only as `std::variant`. Switching types is undefined behavior.
In fact, C++20 has a special casting function, called [`bit_cast`](https://en.cppreference.com/w/cpp/numeric/bit_cast) that solved this problem.
Why has it been working so far? Because some compilers tolerate using unions and switching types, like gcc. More information [here](https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html#Type-punning).
One important thing to mention is that performance is generally not affected by that memcpy. Compilers are smart enough to convert that to a memory cast when possible. But we have to do it the right way, otherwise, it's jut undefined behavior that depends on the compiler.
ACKs for top commit:
practicalswift:
ACK 0653939ac130eddffe40c53ac418bea305d3bf82
elichai:
ACK 0653939ac130eddffe40c53ac418bea305d3bf82
laanwj:
Code review ACK 0653939ac130eddffe40c53ac418bea305d3bf82
kristapsk:
ACK 0653939ac130eddffe40c53ac418bea305d3bf82
Tree-SHA512: f6e89de39fc964750429139bab6b5a1346f7060334b7afa020e315bdad8f8c195bce2b8a9e343f06e7fff175e2dfb1cdabfcb6fe405bea0febe4962f0cc62557
5bad7921d0b33b62c0a59a478c2e8c869fc5e3b5 [test] PSBT RPC: check that bip32_derivs are present by default (Sjors Provoost)
29a21c90610aed88b796a7a5900e42e9048b990e [rpc] set default bip32derivs to true for psbt methods (Sjors Provoost)
Pull request description:
In https://github.com/bitcoin/bitcoin/pull/13557#pullrequestreview-135905054 I recommended not including bip32 deriviation by default in PSBTs:
> _Bit of a privacy issue_: let's say person A and B are about to spend from a multisig address, sending everything to person A. Person A gives their address to person B, their wallet wallet creates a PSBT, but doesn't sign it. Wallet A then calls `walletprocesspsbt` which signs it and _spontaneously adds the master_fingerprint and bip32 path_. Same issue with `walletcreatefundedpsbt`.
>
> Adding `bip32_derivs` should probably be opt-in.
In practice I find this default quite annoying because I forget it and end up with a confused hardware wallet.
More importantly, in the multisig example I provided, it's actually essential for the other side to know the derivation details (in addition to an xpub). This allows them to check that change is going to an address you can still co-sign for (because the spending policy is unchanged except for an index).
ACKs for top commit:
instagibbs:
utACK 5bad7921d0
jonatack:
ACK 5bad7921d0 code review, built, ran tests, inspected/messed around with/pprinted values from the new tests. Thanks for adding the tests.
meshcollider:
utACK 5bad7921d0b33b62c0a59a478c2e8c869fc5e3b5
Tree-SHA512: 22ad71dda96856060a96758c4ae7aafa22d5e9efba30e0c8287c711e7579849bd72593cbc0f41a2e9e8821315d78bda04e848dbb006283b841b2795e2faebcfd
12a2f377185a413b740460db36812de22ee2e041 util: Avoid potential uninitialized read in FormatISO8601DateTime(int64_t nTime) by checking gmtime_s/gmtime_r return value (practicalswift)
Pull request description:
Avoid potential uninitialized read in `FormatISO8601DateTime(int64_t)` by checking `gmtime_s`/`gmtime_r` return value.
Before this patch `FormatISO8601DateTime(67768036191676800)` resulted in:
```
==5930== Conditional jump or move depends on uninitialised value(s)
==5930== at 0x4F44C0A: std::ostreambuf_iterator<char, std::char_traits<char> > std::num_put<char, std::ostreambuf_iterator<char, std::char_traits<char> > >::_M_insert_int<long>(std::ostreambuf_iterator<char, std::char_traits<char> >, std::ios_base&, char, long) const (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25)
==5930== by 0x4F511A4: std::ostream& std::ostream::_M_insert<long>(long) (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25)
==5930== by 0x4037C3: void tinyformat::formatValue<int>(std::ostream&, char const*, char const*, int, int const&) (tinyformat.h:358)
==5930== by 0x403725: void tinyformat::detail::FormatArg::formatImpl<int>(std::ostream&, char const*, char const*, int, void const*) (tinyformat.h:543)
==5930== by 0x402E02: tinyformat::detail::FormatArg::format(std::ostream&, char const*, char const*, int) const (tinyformat.h:528)
==5930== by 0x401B16: tinyformat::detail::formatImpl(std::ostream&, char const*, tinyformat::detail::FormatArg const*, int) (tinyformat.h:907)
==5930== by 0x4017AE: tinyformat::vformat(std::ostream&, char const*, tinyformat::FormatList const&) (tinyformat.h:1054)
==5930== by 0x401765: void tinyformat::format<int, int, int, int, int, int>(std::ostream&, char const*, int const&, int const&, int const&, int const&, int const&, int const&) (tinyformat.h:1064)
==5930== by 0x401656: std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > tinyformat::format<int, int, int, int, int, int>(char const*, int const&, int const&, int const&, int const&, int const&, int const&) (tinyformat.h:1073)
==5930== by 0x4014CC: FormatISO8601DateTime[abi:cxx11](long) (…)
```
The same goes for other very large positive and negative arguments.
Fix by simply checking the `gmtime_s`/`gmtime_r` return value :)
ACKs for top commit:
MarcoFalke:
ACK 12a2f377185a413b740460db36812de22ee2e041
theStack:
re-ACK 12a2f37718
elichai:
re ACK 12a2f377185a413b740460db36812de22ee2e041
Tree-SHA512: 066142670d9bf0944d41fa3f3c702b1a460b5471b93e76a619b1e818ff9bb9c09fe14c4c37e9536a04c99533f7f21d1b08ac141e1b829ff87ee54c80d0e61d48
bca8665d0895c450e552c357a036d9e9579e3678 scripted-diff: Wallet: Rename incorrectly named *UsedDestination (Luke Dashjr)
Pull request description:
These functions are used to mark/check if a key of our own has been used to spend (and only for avoid-reuse wallets), which has nothing to do with the destination/address itself.
Give them more accurate names to avoid confusion.
-BEGIN VERIFY SCRIPT-
sed -i -e 's/UsedDestination/SpentKey/g' $(git grep -l 'UsedDestination' ./src)
-END VERIFY SCRIPT-
ACKs for top commit:
practicalswift:
ACK bca8665d0895c450e552c357a036d9e9579e3678 -- patch looks correct and rationale makes sense
instagibbs:
ACK bca8665d08, much more meaningful name, thanks
kallewoof:
ACK bca8665d0895c450e552c357a036d9e9579e3678
Tree-SHA512: ff13d9061ffa748e92eb41ba962c3ec262a43e4b6abd62408b38c6f650395d6ae5851554257d1900fb02767a88d08380d592a27210192ee9abb72d0945976686
63ce882760e1ceb4114c716851aedaf911ebb93b doc: link to homebrew's troubleshooting page (Gastón I. Silva)
Pull request description:
A trivial documentation update.
When I was following the build steps for mac, I had some errors installing the dependencies. After searching on the Internet, and correcting the errors, I found that `brew doctor` had all the answers I needed. Could have skipped the Internet searches all together.
ACKs for top commit:
fanquake:
ACK 63ce882760e1ceb4114c716851aedaf911ebb93b - a link to the troubleshooting page seems fine. I wouldn't really want our README to have anything more specific than that.
Tree-SHA512: 12c96cd9c9bd39ada21f3f27cbec3ed4bef4b8e74dec7872c892fc6a92a70418a5cc0882ff449883e91d96c01e1ca7104b076590917f397334c82931ec7fda1c
f2472f64604a0c583f950c56e8753d0bee246388 tests: Improve test runner output in case of target errors (practicalswift)
733bbec34fbec85574cc456832b2b2f807e5dce9 tests: Add --exclude integer,parse_iso8601 (temporarily) to make Travis pass until uninitialized read issue in FormatISO8601DateTime is fixed (practicalswift)
5ea81449f30a6fe6db3b6df5e8009f21a782ff44 tests: Add support for excluding fuzz targets using -x/--exclude (practicalswift)
555236f769c13518db70f5df36e5688d63486bd5 tests: Remove -detect_leaks=0 from test/fuzz/test_runner.py - no longer needed (practicalswift)
a3b539a924f8611abb3096f2bd9d35094b5577e3 ci: Run fuzz testing test cases under valgrind (practicalswift)
Pull request description:
Run fuzz testing [test cases (bitcoin-core/qa-assets)](https://github.com/bitcoin-core/qa-assets) under `valgrind`.
This would have caught `util: Avoid potential uninitialized read in FormatISO8601DateTime(int64_t) by checking gmtime_s/gmtime_r return value` (#18162) and similar cases.
ACKs for top commit:
MarcoFalke:
ACK f2472f64604a0c583f950c56e8753d0bee246388 👼
Tree-SHA512: bb0879d40167cf6906bc0ed31bed39db83c39c7beb46026f7b0ee53f28ff0526ad6fabc3f4cb3f5f18d3b8cafdcbf5f30105b35919f4e83697c71e838ed71493
* refactor: add more [[nodiscard]]
* refactor: drop unneeded .get()
* refactor: use functional style int cast
* refactor: use std::numeric_limits::max()
* refactor: use GetRand instead of GetRandInt
* refactor/fix: use the shared_ptr<CWallet> where possible instead of getting the underlying pointer
This is also a fix in regards to the rpcevo.cpp file where the old code could result in a dangling pointer that could result in a crash as the shared_ptr we get back is discarded while the underlying ptr is still being used
* refactor: remove unneeded line
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
