faa6c3d44c861c0486c1369e1d098b7645ab07cd net: Drop only invalid entries when reading banlist.json (MarcoFalke)
Pull request description:
All entries will be dropped when there is at least one invalid one in `banlist.json`. Fix this by only dropping invalid ones.
Also suggested in https://github.com/bitcoin/bitcoin/pull/20966#issuecomment-861150204
ACKs for top commit:
laanwj:
Re-ACK faa6c3d44c861c0486c1369e1d098b7645ab07cd
Tree-SHA512: 5a58e7f1dcabf78d0c65d8c6d5d997063af1efeaa50ca7730fc00056fda7e0061b6f7a38907ea045fe667c9f61d392e01e556b425a95e6b126e3c41cd33deb83
e148a5233292d156cda76cb20afb6641fc20f25e bench: fixed ubsan implicit conversion (Martin Ankerl)
da4e2f1da0388d424659fa8c853fcaf37b4b5959 bench: various args improvements (Jon Atack)
d312fd94a1083cdbf071f2888aab43c62d358151 bench: clean up includes (Jon Atack)
1f10f1663e53474038b9111c4264a250cffe7501 bench: add usage description and documentation (Martin Ankerl)
d3c6f8bfa12f78635752878b28e66cec0c85d4a9 bench: introduce -min_time argument (Martin Ankerl)
9fef8329322277d9c14c8df1867cb3c61477c431 bench: make EvictionProtection.* work with any number of iterations (Martin Ankerl)
153e6860e84df0a3d52e5a3b2fe9c37b5e0b029a bench: change AddrManGood to AddrManAddThenGood (Martin Ankerl)
468b232f71562280aae16876bc257ec24f5fcccb bench: remove unnecessary & incorrect multiplication in MuHashDiv (Martin Ankerl)
eed99cf272426e5957bee35dc8e7d0798aec8ec0 bench: update nanobench from 4.3.4 to 4.3.6 (Martin Ankerl)
Pull request description:
This PR updates the nanobench with the latest release from upstream, v4.3.6. It fixes the missing performance counters.
Due to discussions on #22999 I have done some work that should make the benchmark results more reliable. It introduces a new flag `-min_time` that allows to run a benchmark for much longer then the default. When results are unreliable, choosing a large timeframe here should usually get repeatable results even when frequency scaling cannot be disabled. The default is now 10ms. For this to work I have changed the `AddrManGood` and `EvictionProtection` benchmarks so they work with any number of iterations.
Also, this adds more usage documentation to `bench_bitcoin -h` and I've cherry-picked two changes from #22999 authored by Jon Atack
ACKs for top commit:
jonatack:
re-ACK e148a5233292d156cda76cb20afb6641fc20f25e
laanwj:
Code review ACK e148a5233292d156cda76cb20afb6641fc20f25e
Tree-SHA512: 2da6de19a5c85ac234b190025e195c727546166dbb75e3f9267e667a73677ba1e29b7765877418a42b1407b65df901e0130763936525e6f1450f18f08837c40c
49ee2a0ad88e0e656234b769d806987784ff1e28 Avoid wallet code writing node settings file (Russell Yanofsky)
Pull request description:
Change wallet loading code to access settings through the Chain interface instead of writing settings.json directly. This is for running wallet and node in separate processes, since multiprocess code wouldn't easily work with different processes updating the same file.
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10). The commit was first part of larger PR #10102.
ACKs for top commit:
jamesob:
ACK 49ee2a0ad88e0e656234b769d806987784ff1e28 ([`jamesob/ackr/22217.1.ryanofsky.refactor_avoid_wallet_co`](https://github.com/jamesob/bitcoin/tree/ackr/22217.1.ryanofsky.refactor_avoid_wallet_co))
ryanofsky:
> ACK [49ee2a0](49ee2a0ad8) ([`jamesob/ackr/22217.1.ryanofsky.refactor_avoid_wallet_co`](https://github.com/jamesob/bitcoin/tree/ackr/22217.1.ryanofsky.refactor_avoid_wallet_co))
Zero-1729:
crACK 49ee2a0ad88e0e656234b769d806987784ff1e28
meshcollider:
Code review ACK 49ee2a0ad88e0e656234b769d806987784ff1e28
Tree-SHA512: a81c63b87816f739e02e3992808f314294d6c7213babaafdaaf3c4650ebc97ee4f98f9a4684ce4ff87372df59989b8ad5929159c5686293a7cce04e97e2fabba
4602e09751 chore: bump `MIN_PEER_PROTO_VERSION` to `70216` (Kittywhiskers Van Gogh)
ee4115e734 revert: Only sync mempool from v0.15+ (proto 70216+) nodes (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
Extracted from [dash#6272](https://github.com/dashpay/dash/pull/6272)
## Breaking Changes
None expected.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)**
- [x] I have made corresponding changes to the documentation **(note: N/A)**
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
UdjinM6:
utACK 4602e09751
PastaPastaPasta:
utACK 4602e09751
Tree-SHA512: b3bc821c3e50d3fbd3d2d475cde4ccfca2ebedbaedf7209020c0007ca60a946a4e999775747b0b9615106b053ce0febbeaaf12106dfa810d5f3d2e98f8e73aaf
a02c970eb001b456d74ddc30750fe8b55348ddac qt, refactor: Revert explicit including QStringBuilder (Hennadii Stepanov)
3fd3a0fc87a81d42755246830124833e9ca3f0a9 qt, build: Optimize string concatenation (Hennadii Stepanov)
Pull request description:
From [Qt docs](https://doc.qt.io/qt-5/qstring.html#more-efficient-string-construction):
> ... multiple uses of the \[`QString`\] '+' operator usually means multiple memory allocations. When concatenating n substrings, where n > 2, there can be as many as n - 1 calls to the memory allocator.
With this PR
> ... the '+' will automatically be performed as the `QStringBuilder` '%' everywhere.
The change in the `src/Makefile.qt.include` file does not justify submitting this PR into the main repo, IMHO.
ACKs for top commit:
laanwj:
Code review ACK a02c970eb001b456d74ddc30750fe8b55348ddac
Talkless:
utACK a02c970eb001b456d74ddc30750fe8b55348ddac, built successfully on Debian Sid with Qt 5.15.2, but did not check if any displayed strings are "wrong" after refactoring.
jarolrod:
ACK a02c970eb001b456d74ddc30750fe8b55348ddac
Tree-SHA512: cbb476ee96f27c3bd6e125efab74d8bf24bbdb4c30576b3feea45e203405f3bf5b497dd7d3e11361fc825fcbf4b893b152921a9efdeaf73b42d1865d85f0ae84
71689fe6dc (partial) Merge bitcoin/bitcoin#22981: doc: Fix incorrect C++ named args (fanquake)
2b71a9b030 Merge bitcoin/bitcoin#23755: rpc: Quote user supplied strings in error messages (MarcoFalke)
5a441b38de (partial) Merge bitcoin-core/gui#409: Fix window title of wallet loading window (Hennadii Stepanov)
49c87e93a6 Merge bitcoin/bitcoin#23142: Return false on corrupt tx rather than asserting (W. J. van der Laan)
Pull request description:
backports from bitcoin
ACKs for top commit:
UdjinM6:
utACK 71689fe6dc
knst:
utACK 71689fe6dc
Tree-SHA512: c68e2a1be5669f4fd8b02001ea81310b41fcac2cc5cc660e67b5140b334669c9a071a4bd5b33232580215607f323af5f87218a3465493675a633e112984296eb
073d6d6b2a Merge bitcoin/bitcoin#23723: test: Replace hashlib.new with named constructor (MarcoFalke)
674dcf9a55 Merge bitcoin/bitcoin#23547: Bugfix: RPC/mining: Fail properly in estimatesmartfee if smart fee data is unavailable (MarcoFalke)
546e548755 Merge bitcoin/bitcoin#24153: test: remove unused sanitizer suppressions (fanquake)
78b06a4dd2 Merge bitcoin/bitcoin#23591: refactor: Use underlying type of isminetype for isminefilter (W. J. van der Laan)
Pull request description:
backporting
ACKs for top commit:
UdjinM6:
utACK 073d6d6b2a
knst:
utACK 073d6d6b2a
Tree-SHA512: 5c5af5b795ec86f2b98cf9884e1275b8d3a5e7942f8b6632d74ecb799b1b7fe34071c052ac9af15abac14bad1b886ead5d0478f5e03fe0b461b7b40a7defef9e
4e621037c5 test: move `EXPECTED_STDERR_NO_GOV`{`_PRUNE`} and use it more (Kittywhiskers Van Gogh)
77ce6af5c1 chore: remove ancient setting migration logic (Kittywhiskers Van Gogh)
061aa05cf0 chore: remove old llmq db migration code (Kittywhiskers Van Gogh)
438cb85ece ci: set UBSan to halt on error and provide more information (Kittywhiskers Van Gogh)
3a1743fc7f trivial: avoid unneeded copy when iterating through `mapDenomCount` (Kittywhiskers Van Gogh)
cba650953a test: simplify `pow_test`'s `get_next_work` block index construction (Kittywhiskers Van Gogh)
dacf859218 refactor: make `pdsNotificationInterface` a `unique_ptr`, rename (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
Collection of miscellaneous changes collected from work on earlier pull requests that don't fit into pull requests in the immediate future but are nonetheless useful.
## Checklist
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation **(note: N/A)**
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
knst:
re-utACK 4e621037c5
UdjinM6:
utACK 4e621037c5
Tree-SHA512: 5af4e5afdc34840905ffbf375f53cb12b682053cc135ff190dd02d245da9903a7f3e6af6fb1f1727546bf5034cbe42243477342f775e05d15bcfc5d5957616e9
63b13aa519 merge bitcoin#28525: Drop v2 garbage authentication packet (Kittywhiskers Van Gogh)
662394cd49 merge bitcoin#28489: fix incorrect assumption in v2transport_test (Kittywhiskers Van Gogh)
98782c62df merge bitcoin#28433: Follow-up to BIP324 connection support (Kittywhiskers Van Gogh)
f9825168fb merge bitcoin#28196: BIP324 connection support (Kittywhiskers Van Gogh)
3087275039 merge bitcoin#28419: introduce and use `ConsumePrivateKey` helper (Kittywhiskers Van Gogh)
dccd395a4e merge bitcoin#27577: give seednodes time before falling back to fixed seeds (Kittywhiskers Van Gogh)
eb4f01f931 merge bitcoin#26584: include local ("unreachable") peers in -netinfo table (Kittywhiskers Van Gogh)
10dc874136 merge bitcoin#26982: bitcoin#25880 fixups (Kittywhiskers Van Gogh)
a36f8f2a1a merge bitcoin#25880: Make stalling timeout adaptive during IBD (Kittywhiskers Van Gogh)
1d77f3ff55 merge bitcoin#26519: Add getpeerinfo test for missing version message (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* When backporting [bitcoin#25880](https://github.com/bitcoin/bitcoin/pull/25880), changes needed to be made to `p2p_ibd_stalling.py` to help it pass.
* DIP3 activation had to be delayed to a block beyond the range of the test. This is to prevent block rejection arising from a missing DIP3-compliant coinbase (done with `-dip3params=2000:2000`)
* Mock time was disabled to ensure nodes in the test do not resort to direct fetching (with mock time enabled, nodes would be considered close apart in time, which would prevent the primary node from fetching in parallel, which is behavior this test relies on) (done with `self.disable_mocktime = True`)
* The nodes connected do not report compressed headers support (the test relies on sending `headers` messages and reworking it to use compressed headers has little benefit) (done with `services = NODE_NETWORK | NODE_BLOOM`)
* When backporting [bitcoin#28196](https://github.com/bitcoin/bitcoin/pull/28196), in the `v2transport_test` unit test, references to `4000000` were substituted with `MAX_PROTOCOL_MESSAGE_LENGTH` as Dash messages have a protocol limit of 3MiB ([source](d754799580/src/net.h (L79-L80))) while Bitcoin messages have a protocol limit of 4MB ([source](225718eda8/src/net.h (L62-L63))).
* Additionally note that short message IDs as defined in the BIP324 spec ([source](22660ad307/bip-0324.mediawiki (v2-bitcoin-p2p-message-structure))) have not been changed to include Dash-specific messages, meaning, Dash-specific messages will always take 13 bytes.
* As `FEEFILTER` is not supported by Dash, it has been replaced with a blank string in the short IDs table. It was not removed as doing so would disturb the table's arrangement as specified in spec and require readjustment of tests to account for the change in layout.
## Breaking Changes
None expected.
## Checklist
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation **(note: N/A)**
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
UdjinM6:
utACK 63b13aa519
knst:
utACK 63b13aa519
Tree-SHA512: c41d2c6b1f145be513e285c8f91d00ac31fff4e4d24f611a1fdab24c8740f453b0bb28912021cdf8be4f5ce93dcff8579864727ee14b8e45894b56df524ab48d
It significantly improve speed of forks activation because reduces overhead for block generations
Bigger batch size can cause time-outs for RPC for tsan job (time-out is 30 seconds)
48c7f98b1a doc: drop trailing whitespace (pasta)
697743d77b test: add missing import (UdjinM6)
cfe99fd289 docs: add release notes for 6239 (pasta)
a6bbaacfaa fix: GetHeadersLimit is used for getheaders(2) and headers(2), refactor it to accept `compressed` instead of `msg_type` (UdjinM6)
b224f3f6ca bump p2p_version in tests (PastaPastaPasta)
b423f42aae refactor: sort imports (UdjinM6)
f6c68ba71b refactor: simplify _compute_requested_block_headers (UdjinM6)
07876b2c4a use `MAX_HEADERS_UNCOMPRESSED_RESULT` not `MAX_HEADERS_UNCOMPRESSED_RESULTS` ; use `MAX_HEADERS_UNCOMPRESSED_RESULT` in RPC to avoid breaking changes (pasta)
b137280df4 change to _COMPRESSED or _UNCOMPRESSED (pasta)
303bc7af99 fix: increase it for headers2 only (UdjinM6)
e23410ffdd trivial: rename `MAX_HEADERS_RESULTS_NEW` to `MAX_HEADERS_RESULTS` (Kittywhiskers Van Gogh)
bcf0320691 trivial: move the headers limit determination to `GetHeadersLimit()` (Kittywhiskers Van Gogh)
993c7c0f90 feat: increase the number of block headers able to be downloaded at once to 8000 in protocol version `70234` (pasta)
Pull request description:
## Issue being fixed or feature implemented
We did some testing quite a while ago that found that sending 8000 headers at a time could speed stuff up. But we wanted to wait until compressed headers were implemented. Well, they've been implemented!
## What was done?
Bump 2000 -> 8000 triggered by protocol version
## How Has This Been Tested?
Hasn't, we should setup a few nodes running this and sync them from each other
## Breaking Changes
New protocol version, not breaking but should add notes? I should probably add release notes
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that apply._
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
UdjinM6:
light ACK 48c7f98b1a
knst:
utACK 48c7f98b1a
Tree-SHA512: 54c68b9496131ab7f32504d44398d776a151df809d0120d093bbabb18904a783bd9b58796820209f5d75552df5476e30eaa09d68f7c5057882f94b5766a64f4c
f44edde8fe tests: use only 2 MN and 2 Evo nodes in feature_asset_locks.py to be sure that is enough (Konstantin Akimov)
8286bdf611 fix: assert in signing_shares - amount of members can match with amount of attempts (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Currently we have several quorums which have size 3 with threshold 2 nodes: `llmq_test_instantsend`, `llmq_test_platform`, `llmq_test` and they are used on RegTest.
For extreme case when only 2 nodes exist the assert happens:
```
AssertionError: Unexpected stderr dashd: llmq/signing_shares.cpp:812: static CDeterministicMNCPtr llmq::CSigSharesManager::SelectMemberForRecovery(const llmq::CQuorumCPtr&, const uint256&, size_t): Assertion `size_t(attempt) < quorum->members.size()' failed.
Posix Signal: Aborted
0#: (0x5BF40CE70DA2) stl_vector.h:115 - std::_Vector_base<unsigned long, std::allocator<unsigned long> >::_Vector_impl_data::_M_copy_data(std::_Vector_base<unsigned long, std::allocator<unsigned long> >::_Vector_impl_data const&)
1#: (0x5BF40CE70DA2) stl_vector.h:127 - std::_Vector_base<unsigned long, std::allocator<unsigned long> >::_Vector_impl_data::_M_swap_data(std::_Vector_base<unsigned long, std::allocator<unsigned long> >::_Vector_impl_data&)
2#: (0x5BF40CE70DA2) stl_vector.h:1962 - std::vector<unsigned long, std::allocator<unsigned long> >::_M_move_assign(std::vector<unsigned long, std::allocator<unsigned long> >&&, std::integral_constant<bool, true>)
3#: (0x5BF40CE70DA2) stl_vector.h:771 - std::vector<unsigned long, std::allocator<unsigned long> >::operator=(std::vector<unsigned long, std::allocator<unsigned long> >&&)
4#: (0x5BF40CE70DA2) stacktraces.cpp:777 - HandlePosixSignal
5#: (0x7664C9245320) libc_sigaction.c - ???
6#: (0x7664C929EB1C) pthread_kill.c:44 - __pthread_kill_implementation
7#: (0x7664C929EB1C) pthread_kill.c:78 - __pthread_kill_internal
8#: (0x7664C929EB1C) pthread_kill.c:89 - __GI___pthread_kill
9#: (0x7664C924526E) raise.c:27 - __GI_raise
10#: (0x7664C92288FF) abort.c:81 - __GI_abort
11#: (0x7664C922881B) loadmsgcat.c:1177 - _nl_load_domain
12#: (0x7664C923B507) <unknown-file> - ???
13#: (0x5BF40C6E88C8) signing_shares.cpp:823 - llmq::CSigSharesManager::SelectMemberForRecovery(std::shared_ptr<llmq::CQuorum const> const&, uint256 const&, unsigned long)
14#: (0x5BF40C94A285) quorums.cpp:737 - operator()
15#: (0x5BF40C94A514) std_function.h:292 - _M_invoke
16#: (0x5BF40CE082C6) util.cpp:510 - RPCHelpMan::HandleRequest(JSONRPCRequest const&) const
17#: (0x5BF40C89824A) univalue.h:17 - UniValue::operator=(UniValue&&)
18#: (0x5BF40C89824A) server.h:108 - CRPCCommand::CRPCCommand(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, RPCHelpMan (*)())::{lambda(JSONRPCRequest const&, UniValue&, bool)#1}::operator()(JSONRPCRequest const&, UniValue&, bool) const
19#: (0x5BF40C9976F4) std_function.h:591 - std::function<bool (JSONRPCRequest const&, UniValue&, bool)>::operator()(JSONRPCRequest const&, UniValue&, bool) const
20#: (0x5BF40C9976F4) server.cpp:622 - ExecuteCommand
21#: (0x5BF40C99879F) server.cpp:511 - ExecuteCommands
22#: (0x5BF40C99879F) server.cpp:543 - CRPCTable::execute(JSONRPCRequest const&) const
23#: (0x5BF40CB75F24) httprpc.cpp:247 - HTTPReq_JSONRPC
```
Discovered during implementation of https://github.com/dashpay/dash-issues/issues/77
## What was done?
Changed condition in assert, implemented special case of using Nth element from array size N for `SelectMemberForRecovery`, added test for this case.
## How Has This Been Tested?
Improved functional test `feature_asset_locks.py` to test this corner case for quorum `llmq_test_instantsend` and `llmq_test_platform`
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
UdjinM6:
utACK f44edde8fe
PastaPastaPasta:
utACK f44edde8fe
Tree-SHA512: 5e8035960100778820bbbb242a8a354a27bce00c169bbebc4f3867737301dbdd3cc5993a8d806d55bf6eb467bfe1f36443087a271acc227ba7bb301c4f75bb7a
ad35c1ab2f apply suggestions (UdjinM6)
600dcf3dda refactor: make clang-format happy (UdjinM6)
da9b8e038b feat: Split quorum contribution db out of evodb (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
Quorum data is not stored on-chain, it's a temporary data produced during dkg and should not be a part of evodb.
## What was done?
Use new db in `llmq/` to store quorum data, migrate old data to it.
## How Has This Been Tested?
Run tests, run a node on testnet
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
knst:
utACK ad35c1ab2f
PastaPastaPasta:
utACK ad35c1ab2f
Tree-SHA512: b394a53815897f02ec28776c948a32c258ad7f0d9b78fbfd509146a92c8af53e7577d503bc62ae53c40f88aab2c30160fec09f45f0d3447ba64c05c0887676eb
847288df07b45ca535c849e518b22818ab492896 test: fee rate values that cannot be represented as sat/vB (Jon Atack)
06a90fa0381c790f7bde2ab9bf47d2b22acef4a5 rpc: for sat/vB fee rates, limit ParseFixedPoint decimals to 3 (Jon Atack)
0742c7840f03505597fd2de87db97f12597ef667 rpc: enable passing decimals to AmountFromValue, add doxygen (Jon Atack)
8ce3ef57a3e9ad13c0aaa4648e8584241d53592d test: ParseFixedPoint with 3 decimals for sat/vB fee rates (Jon Atack)
b5033275979a2a495b02b25f70cadbdcc8b6eb6a test: type error and out of range fee rates where missing (Jon Atack)
c5fd4344f7fcc257062a610c8ff26ffcc9b53953 test: explicit fee rates with invalid amounts (Jon Atack)
ea6f76b66ecc52360719053489e0ec9f9a673eab test: improve zero-value explicit fee rate coverage (Jon Atack)
Pull request description:
- Improve/close gaps in existing test coverage before making the change
- Enable passing `decimals` to `ParseFixedPoint()` when calling `AmountFromValue()`
- Limit explicit fee rates in sat/vB passed in by users to 3 decimals, and raise otherwise
- Add regression test coverage
Closes#20534.
ACKs for top commit:
MarcoFalke:
review ACK 847288df07b45ca535c849e518b22818ab492896 🔷
Tree-SHA512: c539d07ae9b21c0d6c8ea460beb9c8dad5559445518aace560abc3c05c588907bae189b6fd7602b3b397de4a42356136c3ec6f960d3dcf2d5d16377aef4ab5a2
f09e6b2585200040be2e8ee44fa79b86b1970d70 test: fix off-by-ones in rpc_fundrawtransaction (Jon Atack)
Pull request description:
The variables in these assertions should be the same within each line.
ACKs for top commit:
laanwj:
ACK f09e6b2585200040be2e8ee44fa79b86b1970d70
theStack:
ACK f09e6b2585200040be2e8ee44fa79b86b1970d70
Tree-SHA512: 7ac754eaadd8cb00a725afa55bccbb8de7547dedac9350d79a9a470918245617e075c56a91adc36fb653bbe8a0a325d59b00443155a7e1a81ebf22e4e4cf56d9
f9cd2bfbccb7a2b8ff07cec5f6d2adbeca5f07c3 Rename CoinSelectionParams::effective_fee to m_effective_feerate (Andrew Chow)
bdd0c2934b7f389ffcfae3b602ee3ecee8581acd wallet: Move discard feerate fetching to CreateTransaction (Andrew Chow)
448d04b931f86941903e855f831249ff5ec77485 wallet: Move long term feerate setting to CreateTransaction (Andrew Chow)
e2f429e6bbf7098f278c0247b954ecd3ba53cf37 wallet: Replace nFeeRateNeeded with effective_fee (Andrew Chow)
1a6a0b0dfb90f9ebd4b86d7934c6aa5594974f5f wallet: Use existing feerate instead of getting a new one (Andrew Chow)
Pull request description:
During coin selection, there are various places where we need to have a feerate. We need the feerate for the transaction itself, the discard fee rate, and long term feerate. Fetching these each time we need them can lead to a race condition where two feerates that should be the same are actually different. One particular instance where this can happen is during the loop in `CreateTransactionInternal`. After inputs are chosen, the expected transaction fee is calculated using a newly fetched feerate. If `pick_new_inputs == false`, the loop will go again with the assumption that the fee for the transaction remains the same. However because the feerate is fetched again, it is possible that it actually isn't and this causes coin selection to fail.
Instead of fetching the feerate each time it is needed, we fetch them all at once at the top of `CreateTransactionInternal`, store them in `CoinSelectionParams`, and use them where needed.
While some of these fee rates probably don't need this caching, I've done it for consistency and the guarantee that they remain the same.
Fixes#19229
ACKs for top commit:
glozow:
reACK f9cd2bfbcc
fjahr:
Code review re-ACK f9cd2bfbccb7a2b8ff07cec5f6d2adbeca5f07c3
Xekyo:
tACK f9cd2bfbcc
meshcollider:
Code review + test run ACK f9cd2bfbccb7a2b8ff07cec5f6d2adbeca5f07c3
Tree-SHA512: be83ff64ba473c3cdd3469c812e214659b6e2a9584c22ed2b1595618fce0d4b35d0901e61068cd1069fc1a8fb911db01dd7312d05c3b8cbafbe2504ab7a3e863
6bfbc97d716faad38c87603ac6049d222236d623 test: disallow sendtoaddress/sendmany when private keys disabled (Jon Atack)
0997019e7681efb00847a7246c15ac8f235128d8 Disallow sendtoaddress and sendmany when private keys disabled (Andrew Chow)
Pull request description:
Since `sendtoaddress` and `sendmany` (which use the `SendMoney` function) create and commit a transaction, they should not do anything when the wallet does not have private keys. Otherwise a valid transaction cannot be made.
Fixes#21104
ACKs for top commit:
jonatack:
ACK 6bfbc97d716faad38c87603ac6049d222236d623
meshcollider:
utACK 6bfbc97d716faad38c87603ac6049d222236d623
kristapsk:
ACK 6bfbc97d716faad38c87603ac6049d222236d623. "Error: Private keys are disabled for this wallet" is definitely a better error message than "Insufficient funds" here. Hopefully change of error code from -6 to -4 doesn't break any software using Bitcoin JSON-RPC API.
Tree-SHA512: f277d6b5252e43942d568614032596f2c0827f00cd0cb71e44ffcb9822bfb15a71730a3e3688f31e59ba4eb7d275250c4e65ad4b6b3e96be6314c56a672432fb
6fa72ceb8021c3b5aea62f6cfe92665c29212923 test: add coverage for passing fee rate as a string (Jon Atack)
ce207d6b93d35bc02fcd2dd28f1fd95869261d43 wallet, bugfix: allow send to take string fee rate values (Jon Atack)
Pull request description:
RPC send currently only accepts fee rates as numbers, which is a user-facing bug. It should accept fee rates as an amount, e.g. a string or a number, as documented in its help and like sendtoaddress, sendmany, fundrawtransaction, walletcreatefundedpsbt, and bumpfee. Provide a fix and regression test coverage.
ACKs for top commit:
MarcoFalke:
review ACK 6fa72ceb8021c3b5aea62f6cfe92665c29212923
achow101:
Code review ACK 6fa72ceb8021c3b5aea62f6cfe92665c29212923
promag:
Code review ACK 6fa72ceb8021c3b5aea62f6cfe92665c29212923.
Tree-SHA512: 735f9269cb1b81953764b5283449c0b154bd62de034225be5bcedc515c84faf767fe8fe0741008679fe412922c847b00d116cb11aab775236b779c847ba87167
fa69c2c78455fd0dc436018fece9ff7fc83a180d wallet: Do not treat default constructed types as None-type (MarcoFalke)
fac4e136fa3d0fab7fde900a6be921313e16e7a6 refactor: Change pointer to reference because it can not be null (MarcoFalke)
Pull request description:
Equating `0==None` and `""==None` is confusing, unneeded and undocumented
ACKs for top commit:
jonatack:
ACK fa69c2c78455fd0dc436018fece9ff7fc83a180d
achow101:
ACK fa69c2c78455fd0dc436018fece9ff7fc83a180d
Sjors:
tACK fa69c2c78455fd0dc436018fece9ff7fc83a180d modulo `unset`
Tree-SHA512: c4c8d0ad80c6697621d356a9545caf28ca2facc82bb2fa8e70eceb52372d25f0685237c73688c4b01da0e75d213c77c0d45011a8bdfe81ea783d85f045786dac
9f08780dd7946b63476e9736745131db8e7f4e93 Use the correct incremental fee constant in bumpfee help (Jon Atack)
3f1e10b2b1cd11f7112fbad6355464bd4adbbc5c Update feeRate (BTC/kvB) to fee_rate (sat/vB) in wallet_bumpfee (Jon Atack)
1b3d7009280595108eb22ac1188bc4367804fc5d Allow zero-fee fundrawtxn and walletcreatefundedpsbt calls (Jon Atack)
Pull request description:
- Fixes https://github.com/bitcoin/bitcoin/pull/20305/files#r525406176. A check to raise an error on zero-fee txns was mistakenly extended in a0d4957 from the bumpfee and send{toaddress, many} RPCs to also include fundrawtransaction and walletcreatefundedpsbt. This commit re-overrides zero fee rate checking for these two RPCs, not only for the feeRate (BTC/kvB) arg to return to previous behavior, but also for the new fee_rate (sat/vB) arg. Negative fee rates will still raise "amount out of range" by the MoneyRange check in src/bitcoin-tx.cpp::AmountFromValue.
- Updates a wallet bumpfee test from feeRate (BTC/kvB) to fee_rate (sat/vB)
- Fixes https://github.com/bitcoin/bitcoin/pull/20305/files#r525405363 to use the correct incremental fee rate constant in the bumpfee help (thanks Marco Falke for the catch) and rectifies "1.000 sat/vB sat/vB" in the help to "1.000 sat/vB"
ACKs for top commit:
MarcoFalke:
review ACK 9f08780dd7946b63476e9736745131db8e7f4e93 🐾
promag:
Code review ACK 9f08780dd7946b63476e9736745131db8e7f4e93.
Xekyo:
Code review reACK 9f08780dd7946b63476e9736745131db8e7f4e93.
Tree-SHA512: 413dfb4f23ebaf3d2ef210dd04610a843272e64eabba428699f5de4d646a86ac4911dab66b5e2f5ebea53b76e4be8347ef40824c1592c750d5eaa12579d3cdf6
05e82d86b09d914ebce05dbc92a7299cb026847b wallet: override minfee checks (fOverrideFeeRate) for fee_rate (Jon Atack)
9a670b4f07a6140de809d73cbd7f3e614eb6ea74 wallet: update sendtoaddress, send RPC examples with fee_rate (Jon Atack)
be481b72e24fb6834bd674cd8daee67c6938b42d wallet: use MIN_RELAY_TX_FEE in bumpfee help (Jon Atack)
449b730579566459e350703611629e63e54657ed wallet: provide valid values if invalid estimate mode passed (Jon Atack)
6da3afbaee5809ebf6d88efaa3958c505c2d71c7 wallet: update remaining rpcwallet fee rate units to BTC/kvB (Jon Atack)
173b5b5fe07d45be5a1e5bc7a5df996f20ab1e85 wallet: update fee rate units, use sat/vB for fee_rate error messages (Jon Atack)
7f9835a05abf3e168ad93e7195cbaa4bf61b9b07 wallet: remove fee rates from conf_target helps (Jon Atack)
b7994c01e9a3251536fe6538a22f614774eec82d wallet: add fee_rate unit warnings to bumpfee (Jon Atack)
410e471fa42d3db04e8879c71f8c824dcc151a83 wallet: remove redundant bumpfee fee_rate checks (Jon Atack)
a0d495747320c79b27a83c216dcc526ac8df8f24 wallet: introduce fee_rate (sat/vB) param/option (Jon Atack)
e21212f01b7c41eba13b0479b252053cf482bc1f wallet: remove unneeded WALLET_BTC_KB_TO_SAT_B constant (Jon Atack)
6112cf20d43b0be34fe0edce2ac3e6b27cae1bbe wallet: add CFeeRate ctor doxygen documentation (Jon Atack)
3f7279161347543ce4e997d78ea89a4043491145 wallet: fix bug in RPC send options (Jon Atack)
Pull request description:
This PR builds on #11413 and #20220 to address #19543.
- replace overloading the conf_target and estimate_mode params with `fee_rate` in sat/vB in the sendtoaddress, sendmany, send, fundrawtransaction, walletcreatefundedpsbt, and bumpfee RPCs
- allow non-actionable conf_target value of `0` and estimate_mode value of `""` to be passed to use `fee_rate` as a positional argument, in addition to as a named argument
- fix a bug in the experimental send RPC described in https://github.com/bitcoin/bitcoin/pull/20220#discussion_r513789526 where args were not being passed correctly into the options values
- update the feerate error message units for these RPCs from BTC/kB to sat/vB
- update the test coverage, help docs, doxygen docs, and some of the RPC examples
- other changes to address the excellent review feedback
See this wallet meeting log for more context: http://www.erisian.com.au/bitcoin-core-dev/log-2020-11-06.html#l-309
ACKs for top commit:
achow101:
re-ACK 05e82d8
MarcoFalke:
review ACK 05e82d86b0 did not test and found a few style nits, which can be fixed later 🍯
Xekyo:
tACK 05e82d86b09d914ebce05dbc92a7299cb026847b
Sjors:
utACK 05e82d86b09d914ebce05dbc92a7299cb026847b
Tree-SHA512: a4ee5f184ada53f1840b2923d25873bda88c5a2ae48e67eeea2417a0b35154798cfdb3c147b05dd56bd6608a784e1b91623bb985ee2ab9ef2baaec22206d0a9c
0be29000c011dec0722481dbebb159873da6fa54 rpc: update conf_target helps for correctness/consistency (Jon Atack)
778b9be40667876c705e586849ea9c9e44cf451c wallet, rpc: fix send subtract_fee_from_outputs help (Jon Atack)
603c0050837ec65765208dd54dde354145fbe098 wallet: add rpc send explicit fee rate coverage (Jon Atack)
dd341e602d5160fc621c0299179b91403756b61d wallet: add sendtoaddress/sendmany explicit fee rate coverage (Jon Atack)
44e7bfa60313e4ae67da49e5ba4535038b71b453 wallet: add walletcreatefundedpsbt explicit fee rate coverage (Jon Atack)
6e1ea4273e52fdcd86c87628aa595c03a071ca8c test: refactor for walletcreatefundedpsbt fee rate coverage (Jon Atack)
3ac7b0c6f1c68e74a84d868a454f508bada6b09d wallet: fundrawtx fee rate coverage, fixup ParseConfirmTarget() (Jon Atack)
2d8eba8f8425a2515022d51f1f5b4911329fbf55 wallet: combine redundant bumpfee invalid params and args tests (Jon Atack)
1697a40b6f841a54ee0d9744ed7fd09034b0ddad wallet: improve bumpfee error/help, add explicit fee rate coverage (Jon Atack)
fc5721723d34f76f9e1ffd2e31f274ea6b22f894 wallet: fix SetFeeEstimateMode() error message (Jon Atack)
052427eef1c9da84c474c5161b1910d3328ef0da wallet, bugfix: fix bumpfee with explicit fee rate modes (Jon Atack)
Pull request description:
Follow-up to #11413 providing a base to build on for #19543:
- bugfix for `bumpfee` raising a JSON error with explicit feerates, fixes issue #20219
- adds explicit feerate test coverage for `bumpfee`, `fundrawtransaction`, `walletcreatefundedpsbt`, `send`, `sendtoaddress`, and `sendmany`
- improves a few related RPC error messages and `ParseConfirmTarget()` / error message
- fixes/improves the explicit fee rate information in the 6 RPC helps, of which 2 were also missing `conf_target` sat/B units
This provides a spec and regression coverage for the potential next step of a universal `sat/vB` feerate argument (see #19543), as well as immediate coverage and minimum fixes for 0.21.
ACKs for top commit:
kallewoof:
Concept/Tested ACK 0be29000c011dec0722481dbebb159873da6fa54
meshcollider:
Code review + functional test run ACK 0be29000c011dec0722481dbebb159873da6fa54
Tree-SHA512: efd965003e991cba51d4504e2940f06ab3d742e34022e96a673606b44fad85596aa03a8c1809f06df7ebcf21a38e18a891e54392fe3d6fb4d120bbe4ea0cf5e0
44cc75f80ee7805a117e9298a182af1a44bcbff4 wallet: error if an explicit fee rate was given but the needed fee rate differed (Karl-Johan Alm)
Pull request description:
This ensures that the code doesn't silently ignore too low fee reates. It will now trigger an error in the QT client, if the user provides a fee rate below the minimum, and becomes a necessary check for #11413.
ACKs for top commit:
Sjors:
utACK 44cc75f80ee7805a117e9298a182af1a44bcbff4 (rebased)
fjahr:
re-ACK 44cc75f80ee7805a117e9298a182af1a44bcbff4
Tree-SHA512: cd5a60ee496e64f7ab37aaa53f7748a7393357b1629ccd9660839d366c6191b6413b871ce3aa7293fce1539336222c300ef6f86304f30a1ae8fe361b02310483
5cc783f5f32ffd550d1b298fc2e9cf6c0439f9fe qt: ensure translator comments end in full stop (Jarol Rodriguez)
Pull request description:
This is a follow-up to #318 which addresses this [nit](https://github.com/bitcoin-core/gui/pull/318#discussion_r706856893) by addressing it globally.
This ensures that all GUI translator comments end in a full stop. If a comment does not end in a full stop, a translator may think that the rest of the comment is being cut off.
While here, add a colon to the word "see" for any comments touched which point to look at a link.
ACKs for top commit:
hebasto:
ACK 5cc783f5f32ffd550d1b298fc2e9cf6c0439f9fe, I have reviewed the code and it looks OK, I agree it can be merged.
shaavan:
Code Review ACK 5cc783f5f32ffd550d1b298fc2e9cf6c0439f9fe
Tree-SHA512: 67a1d56175c974e0af9b460fa44163f7ce139a7b81cfaf8ed2c0e7fb6d5120957c3135d96010aeb6229689468e36673fe9571b5a8c3e1c07e047aba1bd563444
fa1b63c01887adff83f16b1bbba3bd159dc51104 test: Replace hashlib.new with named constructor (MarcoFalke)
Pull request description:
A small refactor that doesn't matter too much, but it using the named constructor is nice because:
* It clarifies that it is a built-in function
* It is (trivially) faster and less code.
ACKs for top commit:
Zero-1729:
ACK fa1b63c01887adff83f16b1bbba3bd159dc51104
w0xlt:
ACK fa1b63c
Tree-SHA512: d23dc4552c1e6fc1f90f8272e47e4efcbe727f0b66a6f6a264db8a50ee6cb6d57a2809befcb95fda6725136672268633817a03dd1859f2298d20e3f9e0ca4a7f
cd8d156354ed32a215de5eab5c394a1d74d91ed4 Bugfix: RPC/mining: Fail properly in estimatesmartfee if smart fee data is unavailable (Luke Dashjr)
Pull request description:
Fixes a regression introduced by #22722
(Not entirely sure on the solution)
ACKs for top commit:
prayank23:
crACK cd8d156354
darosior:
utACK cd8d156354ed32a215de5eab5c394a1d74d91ed4
kristapsk:
utACK cd8d156354ed32a215de5eab5c394a1d74d91ed4
Tree-SHA512: eb4aa3cc345c69c44ffd5733b51b90eefe1d7854b7a2855e8cbb98268db24d43b7d0ae9fbb0eccf9b6dc01da644d19433cc77fec52ff67bf890be1fc53a67fc4
