fa76d8d4d71d844e217686881d4f630eac3a8e10 test: Actually print TSan tracebacks (MarcoFalke)
Pull request description:
Commit 5e5138a721738f47053d915e4c65f925838ad5b4 made the TSan logs to be printed before returning an error from the ci script.
However, it seems that on Cirrus CI, the `--failfast` option will kill not only all python process and bitcoind child process, but also the parent CI bash script, rendering the `trap` inefficient. I believe this bug was introduced in commit 451b96f7d2796d00eabaec56d831f9e9b1a569cc.
ACKs for top commit:
fanquake:
utACK fa76d8d4d71d844e217686881d4f630eac3a8e10
Tree-SHA512: 686f889d38a343882cb62ad6e0c2080196330e7cc7086891a7ff66d9443b455c82ba8d7e4a5cc42daa0513b0ad2743055bfe90e2f6ac88a910ee3b663fabddcd
2b6dd4e75b3ad2daff553fde018fe4c8f1187878 test: use MiniWallet for mempool_package_onemore.py (Sebastian Falbesoner)
eb3c5c4ef2eeb1d37d729d4487ed067a24cf81c8 test: MiniWallet: add helper methods `{send,create}_self_transfer_multi` (Sebastian Falbesoner)
Pull request description:
This PR enables one more of the non-wallet functional tests (mempool_package_onemore.py) to be run even with the Bitcoin Core wallet disabled by using the MiniWallet instead, as proposed in https://github.com/bitcoin/bitcoin/issues/20078. For this purpose helper methods `MiniWallet.{create,send}_self_transfer_multi` are introduced which serve as a replacement for `chain_transaction`. With this, it should be also quite straight-forward to change the larger related test `mempool_packages.py` to use MiniWallet.
ACKs for top commit:
MarcoFalke:
ACK 2b6dd4e75b3ad2daff553fde018fe4c8f1187878 💾
Tree-SHA512: 0c97fa0519ca5eaa6df8953a04678aa8a6a66905a82db6ff40042a675d0c0682aee829a48db84e4e7983d8f766875021f0d39d65e12889342610b8861bc29cd5
2726b60a3ac098b44f2970bed21147b70e12a1c2 test: use MiniWallet for rpc_createmultisig.py (Ayush Sharma)
Pull request description:
This PR enables one of the non-wallet functional tests (rpc_createmultisig.py) to be run even with the Bitcoin Core wallet disabled by using the MiniWallet instead, as proposed in #20078 .
ACKs for top commit:
danielabrozzoni:
re-ACK 2726b60a3ac098b44f2970bed21147b70e12a1c2
Tree-SHA512: fb0ef22d3f1c161ca5963cb19ce76533ac3941f15102fc0aa2286ef3bec48f219e5934d504b41976f9f295fb6ca582b737e0fea896df4eb964cdaba1b2c91650
061accfddd2d27ad584c826413c68857d2be9ced build: require libtool 2.4.2 (fanquake)
Pull request description:
Every system we support has 2.4.6 available, except for OpenBSD, which
[currently ships with 2.4.2](https://github.com/openbsd/ports/blob/master/devel/libtool/Makefile) (released 2011). For now, set our minimum
required version to that.
After a 7 year hitus, 2.4.7 has also very recently been released:
https://savannah.gnu.org/forum/forum.php?forum_id=10139.
Partially motivated by comments in https://github.com/bitcoin/bitcoin/pull/24615.
See also: https://repology.org/project/libtool/versions
ACKs for top commit:
achow101:
ACK 061accfddd2d27ad584c826413c68857d2be9ced
hebasto:
ACK 061accfddd2d27ad584c826413c68857d2be9ced
prusnak:
ACK 061accfddd2d27ad584c826413c68857d2be9ced
Tree-SHA512: bc032022b8609b73253ff1c4fd480f4d09be761b8fec295f39319f9499ee2df116f55295da476be551c43ed88fbb0bfed7bb5a188b9979b34147fe39737ec76f
This makes code that uses the helper less verbose.
Moreover, this makes net_processing C++20 compliant. Otherwise, it would
lead to a compile error (see below). C++20 disables aggregate
initialization when any constructor is declared. See
http://open-std.org/JTC1/SC22/WG21/docs/papers/2018/p1008r1.pdf
net_processing.cpp:1627:42: error: no matching constructor for initialization of 'CSerializedNetMsg'
m_connman.PushMessage(pnode, CSerializedNetMsg{ser_cmpctblock.data, ser_cmpctblock.m_type});
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Without the changes, g++ will warn to compile under C++20:
scheduler.cpp:114:21: warning: implicit capture of ‘this’ via ‘[=]’ is deprecated in C++20 [-Wdeprecated]
114 | scheduleFromNow([=] { Repeat(*this, f, delta); }, delta);
| ^
scheduler.cpp:114:21: note: add explicit ‘this’ or ‘*this’ capture
39b1763730177cd7d6a32fd9321da640b0d65e0e Replace use of `ArgsManager` with `DatabaseOptions` (Kiminuo)
Pull request description:
Contributes to #21005.
The goal of this PR is to remove `gArgs` from database classes (i.e. `bdb.h` and `sqlite.h`) so that they can be tested without relying on `gArgs` in tests.
Notes:
* My goal is to enable unit-testing without relying on `gArgs` as much as possible. Global variables are hard to reason about which in turn makes it slightly harder to contribute to this codebase. When the compiler does the heavy lifting for us and allows us only to construct an object (or call a method) with valid parameters, we may also save some time in code reviews. The cost for this is passing an argument which is not for free but the cost is very miniscule compared to benefits, I think.
* GUI code is an exception because it seems fine to have `gArgs` there so I don't plan to make changes in `src/qt` folder, for example.
* My approach to removal of `gArgs` uses is moving from lower levels to upper ones and pass `ArgsManager` as an argument as needed. The approach is very similar to what #20158.
ACKs for top commit:
achow101:
ACK 39b1763730177cd7d6a32fd9321da640b0d65e0e
ryanofsky:
Code review ACK 39b1763730177cd7d6a32fd9321da640b0d65e0e. Just the two small ReadDatabaseArgs and Berkeley open changes that were discussed since the last review
Tree-SHA512: aa066b314db593e46c18698fe8cdd500f558b405dc04e4a9a3ff57b52b5b3a81a6cb090e0e661785d1d02c1bf18958c1f4cd715ff233aab63381e3f80960622d
fae5d06eed7f766926b1dc6d2320a68c8e4375bc Remove unused feebumper code (MarcoFalke)
Pull request description:
This was accidentally added in commit 0ea47ba7b38cc4b2b9175347cb5cd48fcd08da48. Presumably due to a copy-paste error, as `CreateTransaction` already takes care of the rbf-signal.
ACKs for top commit:
achow101:
ACK fae5d06eed7f766926b1dc6d2320a68c8e4375bc
promag:
Code review ACK fae5d06eed7f766926b1dc6d2320a68c8e4375bc
Tree-SHA512: 81aaf9c6bd9a4e2ad1789880bd8f2191f0ae9ba0a02794aa5db523236ea7df1c0dca078563219d293c694373c0a63c5bf168a85443e86556453ae5439791a618
fa7a576391ad5d61af937dd62496ded44105c671 test: Run non-wallet tests only once (MarcoFalke)
Pull request description:
I don't see why non-wallet tests should run for two wallet configs, even though they never use a wallet.
ACKs for top commit:
achow101:
ACK fa7a576391ad5d61af937dd62496ded44105c671
Tree-SHA512: 2a135acf3c3c83a2704ae11f40c72882b23a676828647be1a066653c4d00e4523704f377eb8745c6386829601cc5d643abdce376831c1db91a07e999e1d5e01f
fa2d176016683eac82dabfbfc276b8a7b07b7499 Move txoutproof RPCs to txoutproof.cpp (MarcoFalke)
Pull request description:
The txoutproof RPCs don't really fit into `rawtransaction.cpp`, as they deal with txids, not with raw transactions. As they are placed in the `blockchain` RPC category, they could be moved there. However, `blockchain.cpp` already takes about 20 seconds to compile (and `rawtransaction.cpp` even longer), so move them to a separate file.
Can be reviewed with `--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`.
ACKs for top commit:
achow101:
ACK fa2d176016683eac82dabfbfc276b8a7b07b7499
theStack:
Concept and code-review ACK fa2d176016683eac82dabfbfc276b8a7b07b7499
Tree-SHA512: 6250e5f87b6237f604d69643f9a809b238702d73f041792c537aeadeafdb60ab8e0dca1d83347d0d6c85900ce179df14365ae303ca3930ed33a528a862f85aa3
These operators are used only by the tests in std::mismatch. As
std::mismatch can take a binary predicate, we can use a lambda that
achieves the same instead.
Instead of having a pointer to the CWalletTx in COutput, we can just
store the COutPoint and the CTxOut as those are the only things we need
from the CWalletTx. Other things CWalletTx used to provide were time and
fIsFromMe but these are also being stored by COutput.
fa7deaa0464576a229b5a6ab13ad033c16d0dada wallet: Pass FastRandomContext& to coin selection (MarcoFalke)
77773b061cb13229a8afb46f6f3ab89fc70eabe3 wallet: Pass FastRandomContext& to DiscourageFeeSniping (MarcoFalke)
Pull request description:
Passing around a single randomness context shouldn't come with any downsides, but documents better where randomness is used and allows the unit test to be deterministic, if they wish to be so.
ACKs for top commit:
achow101:
ACK fa7deaa0464576a229b5a6ab13ad033c16d0dada
promag:
Code review ACK fa7deaa0464576a229b5a6ab13ad033c16d0dada.
glozow:
light code review ACK fa7deaa0464576a229b5a6ab13ad033c16d0dada
Tree-SHA512: c16287708cc82ce58311710595d0127af42fb156c93fbcaa5bde634ce323d325f4d8c99a74af24423ab22b5ad58163dd771e8b1a0e7d6bff39c9fb2a1cb21bc7
7f90dc26c8938f348938929b6d8bf1ea6f149209 options: flip listenonion to false if not listening (Vasil Dimov)
Pull request description:
If the user has unchecked "Allow incoming connections" in
`Settings->Options...->Network` then `fListen=false` is saved in
`~/.config/Bitcoin/Bitcoin-Qt.conf`. This flips `-listen` to `false`
during startup, but leaves `-listenonion` to `true`.
This flipping of `-listen` is done in `OptionsModel::Init()` after
`InitParameterInteraction()` has been executed which would have flipped
`-listenonion`, should it have seen `-listen` being `false`
(this is a difference between `bitcoind` and `bitcoin-qt`).
Fixes: https://github.com/bitcoin-core/gui/issues/567
ACKs for top commit:
mzumsande:
Tested ACK 7f90dc26c8938f348938929b6d8bf1ea6f149209
hebasto:
ACK 7f90dc26c8938f348938929b6d8bf1ea6f149209
jonatack:
utACK 7f90dc26c8938f348938929b6d8bf1ea6f149209
ryanofsky:
Code review ACK 7f90dc26c8938f348938929b6d8bf1ea6f149209.
Tree-SHA512: ff5095096858eae696293dc58d1cd5bd1bb60ef7c5d07d87308a0cf71c67da88cc00b301b550704625f136c4ba3a29905a934a766535a6422fe85d9662299d32
If the user has unchecked "Allow incoming connections" in
`Settings->Options...->Network` then `fListen=false` is saved in
`~/.config/Bitcoin/Bitcoin-Qt.conf`. This flips `-listen` to `false`
during startup, but leaves `-listenonion` to `true`.
This flipping of `-listen` is done in `OptionsModel::Init()` after
`InitParameterInteraction()` has been executed which would have flipped
`-listenonion`, should it have seen `-listen` being `false`
(this is a difference between `bitcoind` and `bitcoin-qt`).
Fixes: https://github.com/bitcoin-core/gui/issues/567
38a1b0b1967272cbb984f37bcc86b6640930319f doc: remove unneeded documentation on basic package management on FreeBSD (jessebarton)
Pull request description:
In reference to #24618
ACKs for top commit:
fanquake:
ACK 38a1b0b1967272cbb984f37bcc86b6640930319f - Thanks. In future, please re-use existing PRs, so that discussion and changes are kept together.
Tree-SHA512: ece5b85bca7f11e11d47c0674a6b96a72c3bb65dd02ab25553db511a001a9fc682c0ff8276e39d979fdd1f57a64137f586cfa548aab5c08cd9341455217b9181
9b526727000509dc6ef90f2ce6a9049edebf959c For descriptor pubkey parse errors, include context information (Ben Woosley)
Pull request description:
This adds readily-available context information to the error string, for further disambiguation.
This is a revival of #16123 which was largely addressed in #16542.
Note 'Multi:' is used rather than 'multi():' as it also encompasses 'sortedmulti():'
ACKs for top commit:
achow101:
ACK 9b526727000509dc6ef90f2ce6a9049edebf959c
theStack:
ACK 9b526727000509dc6ef90f2ce6a9049edebf959c
Tree-SHA512: 96533ea8c3ac7010f9b62e75b4bd20b65aff843030eb91c7a88312975acecaaf17909b7d1841f45edc86dbf7fa402d208adb85f0673bd79b857dbebacb8c9395
acd98adaf1d83b71eda235c49d41a92f30c16313 qt: Avoid potential -Wdeprecated-enum-enum-conversion warning (Hennadii Stepanov)
d8641f04e4350755e5264e55730392730ab7c9ee qt: Use human-readable strings in preference to hard-coded integers (Hennadii Stepanov)
Pull request description:
This PR is related to bitcoin/bitcoin#24169. It adjusts code in order to avoid `-Wdeprecated-enum-enum-conversion` warnings instead of disabling them.
Could be tested with gcc 11.2.
ACKs for top commit:
MarcoFalke:
Approach ACK acd98adaf1d83b71eda235c49d41a92f30c16313
fanquake:
untested ACK acd98adaf1d83b71eda235c49d41a92f30c16313 - thanks.
promag:
Code review ACK acd98adaf1d83b71eda235c49d41a92f30c16313.
Tree-SHA512: e8043d997d85f8dba0f37ca02f1c60eb756a1732cf76a75908b01eb2cf7a4c6d4aaf6007271a929c213de37a0c1d96bc25280f0ee9eca488f370904461222ede
Adapted in part from the CJDNS description in #23077 by Vasil Dimov
and from CJDNS documentation and feedback by Caleb James DeLisle.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
b2774fc0bed53dfaf98206d353d42c474c5bbb1a torcontrol: Query Tor for correct -onion configuration (Luke Dashjr)
Pull request description:
Currently, we just assume any running Tor instance provides localhost port 9050 for SOCKS, and configure `-onion` accordingly when we get a Tor control connection.
This actually queries the Tor node for its SOCKS listeners, and uses the configured port instead.
For backward compatibility, it falls back to localhost:9050 if it can't get any better port info. I'm not sure if that's the correct action to take when the Tor daemon explicitly says there are no ports listening...
ACKs for top commit:
laanwj:
Tested ACK (FreeBSD) b2774fc0bed53dfaf98206d353d42c474c5bbb1a
vasild:
ACK b2774fc0bed53dfaf98206d353d42c474c5bbb1a
jonatack:
ACK b2774fc0bed53dfaf98206d353d42c474c5bbb1a review, rebased to master, debug build, ran unit tests, tested happy path only
Tree-SHA512: 2fa93a3cf0cb675801d1b51322ce953ea9b2317f78154a53b603244d74252f434cc1eaa5ae48cb3fe6bdc4ce984a6d976ff95bb046f7933b9740332942378c02
fa9086d085f664a96561eeb5dd29fc1a4e4f926a test: Limit scope of id global which is shared between subtests (MarcoFalke)
Pull request description:
Globals aren't too nice when testing, as leak state between subtests run in the same process. For example, when checking peer ids in the tests, they might pass/fail depending on other tests run in the same process.
Fix this by making `id` not a global.
ACKs for top commit:
promag:
Code review ACK fa9086d085f664a96561eeb5dd29fc1a4e4f926a.
Tree-SHA512: 0a53dde428570086f4557b23112e6460d6413bedf6ef487bd56e88f83cd5f4526f44effa8076cdeaf4761ecc062c346948e0bff434808bbf9b558eabd81328e3
