a656d2f14f feat: more logging (UdjinM6)
cedd3d52be refactor: make expected_connections optional (UdjinM6)
fd2fbe06a3 fix: check mn state after each mined quorum (UdjinM6)
cce87a662a fix: should have at least 2 connections when testing isolate_mn (UdjinM6)
793f4b739f fix: connect repaired mns only (UdjinM6)
8597acd0dc fix: remember mns that don't listen and avoid them (UdjinM6)
2069625759 fix: calculate expected_complaints correctly (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
Fix some general mistakes and also `connect_nodes` related issues discovered while debugging #6276. Add some logging to make debugging a bit easier.
~NOTE: builds on top of #6278 to avoid conflicts, will rebase~ done
## What was done?
pls see individual commits
## How Has This Been Tested?
run tests
## Breaking Changes
n/a
## Checklist:
- [ ] 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
- [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
kwvg:
LGTM, ACK a656d2f14f
knst:
ACK a656d2f14f
PastaPastaPasta:
utACK a656d2f14f
Tree-SHA512: 30f657218ce0338f9a5a09d9a839cca9c4605740265d2182a1e143ec6ece739fecf748f7b60ccec065c17d9f6d893c0c47893be05c44bb8d34624fb4bf3c2a58
1e17b74207 test: no longer connect nodes in parallel in `start_masternodes` (UdjinM6)
be72ef5592 test: use `setmnthreadactive` to get controlable `connect_nodes` behaviour (UdjinM6)
e2ed82a7ae feat(rpc): introduce `setmnthreadactive` (regtest-only) (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
This adds a new rpc command to enable/disable automatic masternode connections creation. We need this for #6276. 1e17b74207 is extracted from ede1833ba4 to avoid multiple jobs calling `setmnthreadactive` on the same node in parallel.
## What was done?
Add `setmnthreadactive` rpc and use it
## How Has This Been Tested?
run tests
## 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
ACKs for top commit:
kwvg:
LGTM, ACK 1e17b74207
PastaPastaPasta:
utACK 1e17b74207
Tree-SHA512: 83c1c07d0066e26202fd21942a09e41c3560c4d32229b44390946c4acb22319b32aa61a13b9106d20fc8cc197dd2a8ab5fdfcfdeaf3da76af062fc0fd7646972
d6946aaba1 fix: offset fee for 1 duff in commission in wallet_basic.py due to missing bitcoin/bitcoin#22949 (Konstantin Akimov)
3ba99b9c42 Merge bitcoin/bitcoin#21786: wallet: ensure sat/vB feerates are in range (mantissa of 3) (MarcoFalke)
22435f1898 Merge bitcoin/bitcoin#21787: test: fix off-by-ones in rpc_fundrawtransaction assertions (W. J. van der Laan)
ccac35c89c Merge #21083: wallet: Avoid requesting fee rates multiple times during coin selection (Samuel Dobson)
9e9975f83b Merge #21201: rpc: Disallow sendtoaddress and sendmany when private keys disabled (Samuel Dobson)
5ad8a489a5 Merge #20573: wallet, bugfix: allow send with string fee_rate amounts (MarcoFalke)
db4a2169bb Merge #20410: wallet: Do not treat default constructed types as None-type (MarcoFalke)
01e41aa1fb Merge #20426: wallet: allow zero-fee fundrawtransaction/walletcreatefundedpsbt and other fixes (MarcoFalke)
f436c20bc4 Merge #20305: wallet: introduce fee_rate sat/vB param/option (MarcoFalke)
0fa19226cb Merge #20220: wallet, rpc: explicit fee rate follow-ups/fixes for 0.21 (Samuel Dobson)
d530b73016 Merge #18275: wallet: error if an explicit fee rate was given but the needed fee rate differed (MarcoFalke)
Pull request description:
## Issue being fixed or feature implemented
Just regular backports from bitcoin v0.21, v22; mostly wallet+fee related
## What was done?
- bitcoin/bitcoin#18275
- bitcoin/bitcoin#20220
- bitcoin/bitcoin#20305
- bitcoin/bitcoin#20426
- bitcoin/bitcoin#20410
- bitcoin/bitcoin#20573
- bitcoin/bitcoin#21201
- bitcoin/bitcoin#21083
- bitcoin/bitcoin#21787
## How Has This Been Tested?
Run unit and functional tests
## Breaking Changes
Some wallet rpc (sendtoaddress, sendmany, send) have a new argument `fee_rate` which is inserted before `verbose`.
Release notes will be provided in a new PR once scope of backports and fixes in this PR is finalized by merging it to develop/
## Checklist:
- [x] I have performed a self-review of my own code
- [x] 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
ACKs for top commit:
UdjinM6:
utACK d6946aaba1
PastaPastaPasta:
utACK d6946aaba1
Tree-SHA512: 8826e1453fe84e3d21f789fab62c23ea13299ce13a7bf1132f70831c3255e823437b6ddd63f69a8e8a0dae95a2638a4454d727e91177b53a5d331872528b92e8
874ef8cda2 fix: mine_quorum_no_checks -> mine_quorum_less_checks: do some checks to make sure quorums are mined correctly (UdjinM6)
4f636f47b4 fix: re-order functional tests: move governance to 60+seconds category (Konstantin Akimov)
fe49f3f178 refactor: removed dead and commented code from test_framework.py (Konstantin Akimov)
cd1958c82a perf: removed sleep(6) from mine_cycle_quorum in functional tests (Konstantin Akimov)
3f17a01a83 fix: bump mocktime in simplepose when generating blocks to improve robustness (Konstantin Akimov)
132d95e651 perf: remove sleep(1) from each step of quorum creation in functional tests (Konstantin Akimov)
4c57ad1c05 chore: increase batch size from 10 to 50 for faster block generation in functional tests (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Functional tests take too long time to run.
(PR is recreated from https://github.com/dashpay/dash/pull/6268 because CI is broken)
## What was done?
- increased robustness `feature_llmq_simplepose.py` by adding missing bump for mocktime during block generations
- removed sleep(1) from each stage of mine_quorum
- removed sleep(6) from final stage of mine_cycled_quorum
- size of batch for block generation in `feature_asset_locks.py` and in `activate_fork_by_name()` increased from 10 blocks to 50 blocks
- moved governance's functional tests to "60 seconds+" category because they always the last one to wait if running more than 10 jobs at once
Plus extra refactoring which removes dead and commented code from test_framework.py
## How Has This Been Tested?
Locally, the functional tests speed up with these fixes for 15% for overall time and 20% for accumulated time
`test/functional/test_runner.py -j20`
Before:
```
ALL | ✓ Passed | 7860 s (accumulated)
Runtime: 481 s
```
After:
```
ALL | ✓ Passed | 6237 s (accumulated)
Runtime: 416 s
```
---
CI tsan job speeds up for 5 minutes in absolute time (~5%) and 1000 seconds in accumulated time.
```
ALL | ✓ Passed | 23854 s (accumulated)
Runtime: 6249 s
```
↑ [old version](https://gitlab.com/dashpay/dash/-/jobs/7822664869) vs [new version](https://gitlab.com/dashpay/dash/-/jobs/7825461091) ↓
```
ALL | ✓ Passed | 22901 s (accumulated)
Runtime: 5962 s
```
## 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
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
UdjinM6:
utACK 874ef8cda2
PastaPastaPasta:
utACK 874ef8cda2
Tree-SHA512: 514fa2fb32abd59c90f63b68fccc8c3d3b6d16b0b6ad7459c4a348825815e7d3012177565dea1f70b8a1f28ede1a297f91361365454d1be85955e77260451cf5
74a5a9f984 style: apply clang-format (Konstantin Akimov)
db9798f5e4 refactor: move call GetQuorumMembers inside Init() (Konstantin Akimov)
9593566802 refactor: move pQuorumBaseBlockIndex from Init() to constructor in CDKGSession (Konstantin Akimov)
9f3eb6bbb8 perf: check DIP0003 before CDKGSession initialization (Konstantin Akimov)
7f815cb501 refactor: remove unused constructor of CDKGLogger (Konstantin Akimov)
c82672af94 refactor: remove dependency of CDKGSession on PeerManager (Konstantin Akimov)
fb78b0cc94 refactor: remove retBan flag from ReceiveMessage (Konstantin Akimov)
d26d4ab0bc refactor: remove dependency of dkgsessionmgr on dkgsession (Konstantin Akimov)
d361b11e5b refactor: moved including llmq/dkgsession.h from dkgsessionmgr.h to cpp file (Konstantin Akimov)
e77aeb321b refactor: removed including quorums.h from chainlocks.h (Konstantin Akimov)
6f7068ef42 refactor: remove exceeding evodb.h from headers (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
We have 72 circular dependencies of dash specific code. This PR removes 2 of them, over dkgsession.
## What was done?
Refactor dkgsession initialization, message processing, dropped unused arguments, re-distributed code between functions and modules... See each commit.
Also optimized headers: excluded evo/evodb.h and llmq/quorums.h from the headers where they are not needed.
## How Has This Been Tested?
Run `test/lint/lint-circular-dependencies.sh`
Run unit/functional tests
## 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
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
UdjinM6:
utACK 74a5a9f984
PastaPastaPasta:
utACK 74a5a9f984
Tree-SHA512: f3adabe6a7bc6c4dcae6430be6857b31a0722d7f605d6651f3ff93c6fcb350a7312a9a0ecbda8ac131ffef6fbf2499700112de3990c7512ddb057bde7cc42665
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
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
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
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
We need to disable mocktime so that the node doesn't resort to direct
fetching. We also need to delay DIP3's activation so that blocks don't
get rejected for not having a valid DIP3-compliant coinbase.
056d869571 refactor: use testdummy in feature_mnehf functional test, removed useless checks (Konstantin Akimov)
0351469bb5 refactor: removed duplicated meaningless condition from Check mnhftx (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Using mn_rr in feature_mnehf.py is a blocker for burying mn_rr fork.
## What was done?
Removed useless conditions, uses testdummy fork instead mn_rr in ehf functional test.
## How Has This Been Tested?
Run it `test/functional/feature_mnehf.py`
## 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
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
UdjinM6:
utACK 056d869571
PastaPastaPasta:
utACK 056d869571
Tree-SHA512: aa016dc21fee3afdb5a4172e757cdd0b92867c23eb5241287f641d3c97d363ab1c46eba06423481ecc7ba05f6c0010b65eadfe77d4a1fd6074706cc6f3a71309
fa3bdbd37be81b613e48a10aa77dfd3bcede61e1 test: remove unused sanitizer suppressions (MarcoFalke)
Pull request description:
Looks like those are not needed (anymore)
ACKs for top commit:
fanquake:
ACK fa3bdbd37be81b613e48a10aa77dfd3bcede61e1
Tree-SHA512: 4bedb6363aba8ea7763291ee0cd074e6bfd77e691bb32999c3959393864dc396bacba1eced2b10d9d600b66e8b83b91f7bc6692331dbd113bbaa87e72d11e2e8
