bf3be5297a746982cf8e83f45d342121e5665f80 [qa] Ensure we don't generate a too-big block in p2sh sigops test (Suhas Daftuar)
Pull request description:
There's a bug in the loop that is calculating the block size in the p2sh sigops test -- we start with the size of the block when it has no transactions, and then increment by the size of each transaction we add, without regard to the changing size of the encoding for the number of transactions in the block.
This might be fine if the block construction were deterministic, but the first transaction in the block has an ECDSA signature which can be variable length, so we see intermittent failures of this test when the initial transaction has a 70-byte signature and the block ends up being one byte too big.
Fix this by double-checking the block size after construction.
ACKs for top commit:
jonasschnelli:
utACK bf3be5297a746982cf8e83f45d342121e5665f80
jnewbery:
tested ACK bf3be5297a746982cf8e83f45d342121e5665f80
Tree-SHA512: f86385b96f7a6feafa4183727f5f2c9aae8ad70060b574aad13b150f174a17ce9a0040bc51ae7a04bd08f2a5298b983a84b0aed5e86a8440189ebc63b99e64dc
e263a343d4b6a2622df6bb734cd9d51a0d20a663 test: rpc_users: Make variable names more clear. (Carl Dong)
830dc2dd0fccb7f3ec49ff7233a188d92c541e7e test: rpc_users: Also test rpcauth.py with specified password. (Carl Dong)
c73d871799982ca29c29cef90e1a78814cf34019 test: rpc_users: Add function for testing auth params. (Carl Dong)
604e2a997ff26202dd0fa1932d60dc14cc53ac6d test: rpc_users: Add function for auth'd requests. (Carl Dong)
Pull request description:
Fixes#14758
First two commits are tidy-ups which I feel are worthwhile as they are very straightforward, cut down the file by 50%, and made the final diff more minimal. Happy to squash after review.
ACKs for top commit:
laanwj:
ACK e263a343d4b6a2622df6bb734cd9d51a0d20a663
Tree-SHA512: aa75c48570a87060238932d4c68e17234e158077f6195fb4917367e1ecc565e3cd8dd0ae51f9159ddd3d03742739680391bc1246454302db22d4a608c0633e80
e13fea975d Add regression test for PSBT signing bug #14473 (Glenn Willen)
565500508a Refactor PSBTInput signing to enforce invariant (Glenn Willen)
0f5bda2bd9 Simplify arguments to SignPSBTInput (Glenn Willen)
53e6fffb8f Add bool PSBTInputSigned (Glenn Willen)
65166d4cf8 New PartiallySignedTransaction constructor from CTransction (Glenn Willen)
4f3f5cb4b1 Remove redundant txConst parameter to FillPSBT (Glenn Willen)
fe5d22bc67 More concise conversion of CDataStream to string (Glenn Willen)
Pull request description:
As discussed in the comments on #14473, I think that bug was caused primarily by failure to adhere to the invariant that a PSBTInput always has exactly one of the two utxo fields present -- an invariant that is already enforced by PSBTInput::IsSane, but which we were temporarily suspending during signing.
This refactor repairs the invariant, also fixing the bug. It also simplifies some other code, and removes redundant parameters from some related functions.
fixes#14473
Tree-SHA512: cbad3428175e30f9b7bac3f600668dd1a8f9acde16b915d27a940a2fa6d5149d4fbe236d5808fd590fb20a032274c99e8cac34bef17f79a53fdf69a5948c0fd0
<!--
*** Please remove the following help text before submitting: ***
Provide a general summary of your changes in the Title above
Pull requests without a rationale and clear improvement may be closed
immediately.
Please provide clear motivation for your patch and explain how it
improves
Dash Core user experience or Dash Core developer experience
significantly:
* Any test improvements or new tests that improve coverage are always
welcome.
* All other changes should have accompanying unit tests (see
`src/test/`) or
functional tests (see `test/`). Contributors should note which tests
cover
modified code. If no tests exist for a region of modified code, new
tests
should accompany the change.
* Bug fixes are most welcome when they come with steps to reproduce or
an
explanation of the potential issue as well as reasoning for the way the
bug
was fixed.
* Features are welcome, but might be rejected due to design or scope
issues.
If a feature is based on a lot of dependencies, contributors should
first
consider building the system outside of Dash Core, if possible.
-->
## Issue being fixed or feature implemented
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
Before this fix, uniqueness of HPMN `platformNodeID` was checked only
while processing a block containing a `ProRegTx` or a `ProUpServTx`.
This is not enough as a `ProRegTx` or `ProUpServTx` containing duplicate
HPMN `platformNodeID` must be rejected at tx broadcast level.
## What was done?
<!--- Describe your changes in detail -->
Checking uniqueness when calling respective RPC and when receiving such
txs.
## How Has This Been Tested?
<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran
to -->
<!--- see how your change affects other areas of the code, etc. -->
## Breaking Changes
<!--- Please describe any breaking changes your code introduces -->
## Checklist:
<!--- Go over all the following points, and put an `x` in all the boxes
that apply. -->
- [x] I have performed a self-review of my own code
- [ ] 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
**For repository code-owners and collaborators only**
- [x] I have assigned this pull request to a milestone
## Issue being fixed or feature implemented
`CDeterministicMNList` stores internally a map containing the hashes of
all properties that needed to be unique.
`pubKeyOperator` don't differ between the two schemes (legacy and
basic(v19)) but their serialisation do: hence their hash.
Because this internal map stores only hashes, then we need to
re-calculate hashes and repopulate.
So when we tried to revoke a masternode after the fork, the `ProUpRevTx`
couldn't be mined because the hash of the `pubKeyOperator` differed.
## What was done?
When retrieving a `CDeterministicMNList` for a given block, if v19 is
active for that block, then we repopulate the internal map.
## How Has This Been Tested?
Without this fix, `feature_dip3_v19.py` is failing with
`failed-calc-cb-mnmerkleroot` (Error encountered on Testnet)
## Breaking Changes
## 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
**For repository code-owners and collaborators only**
- [x] I have assigned this pull request to a milestone
---------
Co-authored-by: pasta <pasta@dashboost.org>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
c4027e735072c3de4b4ffb20eecd7187ff36bad7 refactor: test: use wait_for_getdata() in p2p_compactblocks.py (Sebastian Falbesoner)
Pull request description:
The method `wait_for_getdata()` was recently changed to be more precise by waiting for a specified list of hashes, instead of only matching _any_ `getdata` message (see Issue #18614 and PR #18690). This PR replaces the remaining occurences of manual inspection of `last_messages` with this call.
ACKs for top commit:
robot-visions:
ACK c4027e735072c3de4b4ffb20eecd7187ff36bad7
Tree-SHA512: e10b346742f235b6ee2ef1f32f7fd74406c1a277389f020fb9913a93e94cc9530e1e9414872b83c9d2ae652ebce2b09b2c8c8372260c1afb4e0e54fbf7a935b0
faff9e4bb431919a4bc7e4dc4a9ca188e2d18113 test: Remove unused, undocumented and misleading CScript.__add__ (MarcoFalke)
Pull request description:
See the corresponding pull #18612
ACKs for top commit:
laanwj:
ACK faff9e4bb431919a4bc7e4dc4a9ca188e2d18113 provided it passes Travis
Tree-SHA512: 5d9c4d5b6453c70b24a6960d3b42834e9b31f6dbb99ac47a6abfd85f2739d5372563e7188c22aceabeee1c37eb218bf580848356f4a77268d65f178a9419b269
9e1cb1adf1800efe429e348650931f2669b0d2c0 [trivial/doc] Fix comment type (Amiti Uttarwar)
8f30260a67166a6ab7c0f33f7ec1990d3c31761e [doc] Update unbroadcast description in RPC results (Amiti Uttarwar)
750456d6f29c63d57af05bfbdd6035bb9c965de2 [trivial] Remove misleading 'const' (Amiti Uttarwar)
fa32e676e5833a5c5fc735ef00c0a80f5fab7a2c [test] Manage node connections better in mempool persist test (Amiti Uttarwar)
1f94bb0c744a103b633c1051e8fbc01e612097dc [doc] Provide rationale for randomization in scheduling. (Amiti Uttarwar)
9c8a55d9cb0ec73f10b196e79b637aa601c0a6b7 [mempool] Don't throw expected error message when upgrading (Amiti Uttarwar)
ba5498318233ab81decbc585e9619d8ffe2df1b0 [test] Test that wallet transactions aren't rebroadcast before 12 hours (Amiti Uttarwar)
00d44a534b4e5ae249b8011360c6b0f7dc731581 [test] P2P connection behavior should meet expectations (Amiti Uttarwar)
bd093ca15de762fdaf0937a0877d17b0c2bce16e [test] updates to unbroadcast test (Amiti Uttarwar)
dab298d9ab5a5a41685f437db9081fa7b395fa73 [docs] add release notes (Amiti Uttarwar)
Pull request description:
This PR is a follow up to #18038 which introduced the idea of an unbroadcast set & focuses mostly on documentation updates and test fixes. One small functionality update to not throw an expected error in `LoadMempool` when you upgrade software versions.
#18895 is another follow up to that addresses other functionality updates.
Background context:
The unbroadcast set is a mechanism for the mempool to track locally submitted transactions (via wallet or RPC). The node does a best-effort of delivering the transactions to the network via retries every 10-15 minutes until either a `GETDATA` is received or the transaction is removed from the mempool.
ACKs for top commit:
MarcoFalke:
ACK 9e1cb1adf1 👁
gzhao408:
ACK [`9e1cb1a`](9e1cb1adf1)
Tree-SHA512: 0cd51c4ca368b9dce92d50d73ec6e9df278a259e609eef2858f24cb8595ad07acc3db781d9eb0c351715f18fca5a2b4526838981fdb34a522427e9dc868bdaa6
651f1d816f054cb9c637f8a99c9360bba381ef58 [test] wait for inital broadcast before comparing mempool entries (gzhao408)
9d3f7eb9860254eb787ebe2734fd6a26bcf365c1 [mempool] sanity check that all unbroadcast txns are in mempool (gzhao408)
a7ebe48b94c5a9195c8eabd193204c499cb4bfdb [rpc] add unbroadcast info to mempool entries and getmempoolinfo (gzhao408)
d16006960443c2efe37c896e46edae9dca86c57d [wallet] remove nLastResend logic (gzhao408)
Pull request description:
Followup to #18038 by amitiuttarwar which introduces the unbroadcast set: "a mechanism for the mempool to track locally submitted transactions" and decreases the frequency of rebroadcast from 10-15 minutes to 12-36 hours.
This PR addresses some of the outstanding TODOs building on top of it:
- remove `nLastResend` logic, which is used to ensure rebroadcast doesn't happen again if no new block has been mined (makes sense in 10-15 min period, but not necessary for 12-36 hour period). (#18038 [comment](https://github.com/bitcoin/bitcoin/pull/18038#discussion_r416826914))
- expose unbroadcast info via RPCs, for more informative queries and testing (#18038 [comment](https://github.com/bitcoin/bitcoin/pull/18038#discussion_r416837980))
- add sanity check to verify unbroadcast transactions still exist in mempool before rebroadcasting (#18038 [comment](https://github.com/bitcoin/bitcoin/pull/18038#discussion_r416861609))
ACKs for top commit:
naumenkogs:
Code review ACK 651f1d816f054cb9c637f8a99c9360bba381ef58
amitiuttarwar:
ACK 651f1d816f054cb9c637f8a99c9360bba381ef58 🎉
MarcoFalke:
Review ACK 651f1d816f054cb9c637f8a99c9360bba381ef58
Tree-SHA512: d5327e95ef39d44152b48df5c610502ae11c168f43dbbfb2885340c93d1ba9426eb3a5794573f5fc843502109cb3ffb63efa3f2db4f8f112efcde8f76d9a8845
20b6e959449d0c07639599b99ba917d2cac62493 test: refactor functional tests to use restart_node (Christopher Coverdale)
Pull request description:
fixes#19345
This PR replaces consecutive calls to `stop_node()` and `start_node()` with `restart_node()` where appropriate in the functional tests.
The commit messages are repetitive but focused on each file changed with the intention of squashing if applicable.
ACKs for top commit:
laanwj:
ACK 20b6e959449d0c07639599b99ba917d2cac62493
Tree-SHA512: 1cfa1fb8c5f01a7b00fe44e80dbef072147f21e3891098817acd4275b0c5d91dc1c787594209e117edd418f2fa3a7b2dfcbafdf87efc07f740040938d641f3a9
80d4423f997e15780bfa3f91bf4b4bf656b8ea45 Test buffered valid message (Troy Giorshev)
Pull request description:
This PR is a tweak of #19302. This sends a valid message.
Additionally, this test includes logging in the same vein as #19272.
ACKs for top commit:
MarcoFalke:
tested ACK 80d4423f997e15780bfa3f91bf4b4bf656b8ea45 (added an assert(false) to observe deterministic coverage) 🌦
gzhao408:
ACK 80d4423f99👊
Tree-SHA512: 3b1aa5ec480a1661917354788923d64595e2886448c9697ec0606a81293e8b4a4642b2b3cc9afb2206ce6f74e5c6d687308c5ad19cb73c5b354d3071ad8496f8
9a40cfc558b3f7fa4fff1270f969582af17479a5 [refactor] use waiting inside disconnect_p2ps (gzhao408)
aeb9fb414e2d000830287d9dd3fed7fc2eb570d2 [test] wait for disconnect_p2ps to be reflected in getpeerinfo (gzhao408)
e81942d2e1288367e8da94adb2b2a88be99e4751 [test] logging and style followups for bloomfilter tests (gzhao408)
Pull request description:
Followup to #19083 which adds bloomfilter-related tests.
1. Make test_node `disconnect_p2ps` wait until disconnection is complete to avoid race conditions (and not place the burden on tests) from MarcoFalke's [comment](https://github.com/bitcoin/bitcoin/pull/19083#discussion_r437383989). And clean up any redundant `wait_until`s in the functional tests.
2. Clean up style + logging in p2p_filter.py and p2p_nobloomfilter_messages.py and jonatack's other [comments](https://github.com/bitcoin/bitcoin/pull/19083#pullrequestreview-428955784)
ACKs for top commit:
jonatack:
Code review ACK 9a40cfc from re-reviewing the diff and `git range-diff 5cafb46 8386ad5 9a40cfc`
MarcoFalke:
ACK 9a40cfc558b3f7fa4fff1270f969582af17479a5 🐂
Tree-SHA512: 2e14b1c12fc08a355bd5ccad7a2a734a4ccda4bc7dc7bac171cb57359819fc1599d764290729af74832fac3e2be258c5d406c701e78ab6d7262835859b9a7d87
fa156999695ddaeb016d8320bee62f8d96679d55 test: Add basic test for BIP 37 (MarcoFalke)
Pull request description:
This does not add full coverage, but should be a good start and can be extended in the future. Currently, none of the BIP 37 p2p code has test coverage.
ACKs for top commit:
practicalswift:
Code review ACK fa156999695ddaeb016d8320bee62f8d96679d55 -- more testing coverage is better than less testing coverage
Tree-SHA512: d52e8be79240dffb769105c087ae0ae9305d599282546e4ca7379c4c7add2dbcd668265b46670aa07c357638044cf0f61a6fab7dba8971dd0f80c8f99768686e
45eff751c6d07007dabc365dc4c0e6c63e3fe5cf Add functional test for P2P eviction logic of inbound peers (Martin Zumsande)
Pull request description:
This adds a functional test for the eviction logic for inbound peers, which is triggered when the number of maximum connections is exceeded.
The functional test covers eviction protection for peers that have sent us blocks or txns recently, or that have faster pings. I couldn't find a way to test the logic of `CConnman::AttemptToEvictConnection` that is based on netgroup (see #14210 for related discussion)
Fixes#16660 (at least partially).
[Edit: Earlier, this PR also contained a unit test, which was removed after the discussion]
ACKs for top commit:
jonatack:
ACK 45eff751c6d07007dabc365dc4c0e6c63e3fe5cf
naumenkogs:
Tested ACK 45eff75
fjahr:
re-ACK 45eff751c6d07007dabc365dc4c0e6c63e3fe5cf
andrewtoth:
re-ACK 45eff751c6d07007dabc365dc4c0e6c63e3fe5cf
Tree-SHA512: 177208ab6f30dc62da1cc5f51e654f7c9770d8c6b42aca6ae7ecb30e29d3096e04d75739578e7d149a0f29dd92652b4a707e93c0f1be8aa7ed315e6ec3ab07a4
fa98e10d5efcd965ee224ec21c9e79ebb123f055 test: Remove leftover comment in mining_basic (MarcoFalke)
faedb50d89cf113084adfa50c280c295c95571a8 test: pep-8 mining_basic (MarcoFalke)
Pull request description:
Remove an accidental leftover comment from #19082, which no longer applies and thus might be confusing
ACKs for top commit:
adamjonas:
code review ACK fa98e10
Tree-SHA512: c7f7f8f579b3c6e92f45769be0a7af1a421438a3f5524db5278b2269511a9e0e08f44e3836afb26727644035897ee51ff8296d13ce23030549e7403f57b40e40
6fc641644f7193365cf2b40f5cf20374ec871943 change blacklist to blocklist (TrentZ)
Pull request description:
Let's use a more appropriate and clear word and discard the usage of the blacklist. Blocklist is clear and shall make everyone happy.
ACKs for top commit:
amitiuttarwar:
ACK 6fc641644f7193365cf2b40f5cf20374ec871943
jonatack:
ACK 6fc641644f7193365cf2b40f5cf20374ec871943 git grep shows these two lines to be the only uses of the word in the codebase other than for specifying colors for the GUI.
sipsorcery:
ACK 6fc641644f7193365cf2b40f5cf20374ec871943 due to easy change.
Tree-SHA512: 12fd55ad5c79f1a227da90c7fa730972aae6b74ab1f9df79ec1e7d0eca05c383ef7d6ef5f353620a01da344db915005339b62ca0884179d0f47fbefb084c9efc
7daffc6a90a797ce7c365a893a31a31b0206985c [test] CScriptNum Decode Check as Unit Tests (Gillian Chu)
Pull request description:
The CScriptNum test (#14816) is a roundtrip test of the test framework. Thus, it would be better suited as a unit test. This is now possible with the introduction of the unit test module for the functional tests. See #18576.
This PR:
1. Refactors the CScriptNum tests into 2 unit tests, one in script.py and one in blocktools.py.
2. Extends the script.py CScriptNum test to trial larger numbers.
ACKs for top commit:
laanwj:
ACK 7daffc6a90a797ce7c365a893a31a31b0206985c
Tree-SHA512: 17a04a4bfff1b1817bfc167824c679455d9e06e6e0164c00a7e44f8aa5041c5f5080adcc1452fd80ba1a6d8409f976c982bc481d686c434edf97a5893a32a436
f0dfac7da3886f8c1a1eedd70f4fd2bd298b9cd9 test: add executable flag for rpc_estimatefee.py (Sebastian Falbesoner)
Pull request description:
Again a functional test without executable flag set sneaked in (see e.g. https://github.com/bitcoin/bitcoin/pull/17806 and https://github.com/bitcoin/bitcoin/pull/16742 for previous similar PRs, setting the filemode from 644 to 755). Maybe a linter like suggested in https://github.com/bitcoin/bitcoin/pull/17830 would be worth considering to avoid future (trivial) PRs like this?
ACKs for top commit:
promag:
ACK f0dfac7da3886f8c1a1eedd70f4fd2bd298b9cd9.
kristapsk:
ACK f0dfac7da3886f8c1a1eedd70f4fd2bd298b9cd9
Tree-SHA512: b37c11bdef439aa9d5736c9e0e0bbcc19aff876744f0c4e099ca5c67c9ff1293f1f9140f0d167ea13fee5396ae017aa4a0f1bae4f7aec8fa80b46beb421561c1
612a931d1a3ac1678d02aed30c48fd25ccd113db tests: simplify next_block() function in feature_block (John Newbery)
Pull request description:
The solve parameter is unnecessary. Remove it and add comments.
ACKs for top commit:
MarcoFalke:
ACK 612a931d1a3ac1678d02aed30c48fd25ccd113db
TheQuantumPhysicist:
ACK 612a931
Looks good. Thanks for improving it 😄
practicalswift:
ACK 612a931d1a3ac1678d02aed30c48fd25ccd113db -- simpler is better and patch looks correct :)
Tree-SHA512: 25b4879842ea37a3f598be886f02ce4c2fb0b5a618d02b266dbd380f5cbfdd71a8bd35ddd9d6f2cf83920e37c02caf9955a841a02b17ba75ac63f88d32f8b60b
111880aaf7e12a12f0797f1b19673e3d96328edd [test] Add coverage to estimaterawfee and estimatesmartfee (Ben Woosley)
Pull request description:
This adds light functional coverage to estimaterawfee - a subset of
the testing applied to estimatesmartfee, and argument validation
testing to both estimaterawfee and estimatesmartfee.
One valid estimatesmartfee signature test is commented out because it
fails currently.
Extracted from #12940
Top commit has no ACKs.
Tree-SHA512: 361a883457b28b2dc75081666e49d6dc6b5d76eed40d858abe2dd4f35ece152cf1f99c94480a91f42a896aa2a73cf55f57921316fe66970b2d7ba691a3b17e2d
## Issue being fixed or feature implemented
`develop` can't sync on mainnet and testnet atm because platform quorums
are already active there but we skip non-hpms nodes when calculating
quorums.
## What was done?
Fixed the code to respect `IsV19Active`. Also dropped
`IsLLMQTypeHPMNOnly` cause it's not used anywhere else and it just makes
things more confusing imo.
## How Has This Been Tested?
Can successfully sync on mainnet/testnet
## Breaking Changes
n/a, fixes breaking changes introduced earlier :)
## Checklist:
<!--- Go over all the following points, and put an `x` in all the boxes
that apply. -->
- [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
**For repository code-owners and collaborators only**
- [x] I have assigned this pull request to a milestone
## Issue being fixed or feature implemented
## What was done?
1. Increased protocol version of mininode to match v19 changes in
`MNLISTDIFF` P2P message
2. Added verification of MNs and HPMNs (dip4) in `feature_llmq_hpmn.py`
## How Has This Been Tested?
## Breaking Changes
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] 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
**For repository code-owners and collaborators only**
- [x] I have assigned this pull request to a milestone
## Issue being fixed or feature implemented
## What was done?
As discussed with Platform team, threshold for `llmq_test_platform`
needed to be 67%. Therefore, the size went from 4 members to 3 (while
keeping threshold to 2)
## How Has This Been Tested?
## Breaking Changes
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] 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
**For repository code-owners and collaborators only**
- [x] I have assigned this pull request to a milestone
## Issue being fixed or feature implemented
## What was done?
- `masternode status` now returns the type as well
- `masternode count` now returns in addition total and total enabled MNs
per type.
## How Has This Been Tested?
Added functional tests
## Breaking Changes
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] 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
**For repository code-owners and collaborators only**
- [x] I have assigned this pull request to a milestone
## Issue being fixed or feature implemented
## What was done?
Implementation of 4k collateral HPMN.
## How Has This Been Tested?
## Breaking Changes
## 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
**For repository code-owners and collaborators only**
- [x] I have assigned this pull request to a milestone
---------
Co-authored-by: thephez <thephez@users.noreply.github.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
Co-authored-by: pasta <pasta@dashboost.org>
Co-authored-by: PastaPastaPasta <6443210+pastapastapasta@users.noreply.github.com>
Co-authored-by: UdjinM6 <1935069+Udjinm6@users.noreply.github.com>
Co-authored-by: Konstantin Akimov <545784+knst@users.noreply.github.com>
fa499b5f027f77c0bf13699852c8c06f78e27bef rpc: bugfix: Properly use iswitness in converttopsbt (MarcoFalke)
fa5c5cd141f0265a5693234690ac757b811157d8 rpc: Switch touched RPCs to IsValidNumArgs (MarcoFalke)
Pull request description:
When a serialized transaction has inputs, there is no risk in only trying to deserialize it with witness allowed. (This is how all transactions from p2p are deserialized.) In fact, it would avoid a common issue where a transaction with inputs can be deserialized in two ways:
* Fixes#12989
* Fixes#15872
* Fixes#15701
* Fixes#13738
* ...
When a serialized transaction has no inputs, there is no risk in only trying to deserialze it with witness disallowed. (A transaction without inputs can't have corresponding witness data)
ACKs for commit fa499b:
meshcollider:
utACK fa499b5f02
ryanofsky:
utACK fa499b5f027f77c0bf13699852c8c06f78e27bef. Changes since last review: consolidating commits and making iswitness documentation the same across methods.
PastaPastaPasta:
utACK fa499b5f027f77c0bf13699852c8c06f78e27bef
Tree-SHA512: a64423a3131f3f0222a40da557c8b590c9ff01b45bcd40796f77a1a64ae74c6680a6be9d01ece95c492dfbcc7e2810409d2c2b336c2894af00bb213972fc85c6
7813eb1db1 [qa] Overhaul p2p_compactblocks.py (Suhas Daftuar)
+ extra fixes for pull request #1966 (compact blocks)
Pull request description:
Remove tests of:
- compactblock behavior in a simulated pre-segwit version of bitcoind
This should have been removed a long time ago, as it is not generally
necessary for us to test the behavior of old nodes (except perhaps if we
want to test that upgrading from an old node to a new one behaves properly)
- compactblock behavior during segwit upgrade (ie verifying that network
behavior before and after activation was as expected)
This is unnecessary to test now that segwit activation has already happened.
ACKs for commit 7813eb:
jnewbery:
utACK 7813eb1db132c023902ad945995cc32a325893ca
Tree-SHA512: cadf035e6f822fa8cff974ed0c2e88a1d4d7da559b341e574e785fd3d309cc2c98c63bc05479265dc00550ae7b77fc3cbe815caae7f68bcff13a04367dca9b52
b651ef7e1c submitheader: more directly test missing prev block header (Gregory Sanders)
1e7f741745 remove some magic mining constants in functional tests (Gregory Sanders)
Pull request description:
The fewer magic numbers the better.
Also more directly tested a `submitheader` case of bad previous blockhash.
Tree-SHA512: 52b01a6aa199fa909eea4e9e84409a901933e545724e33149cc4132c82168199fd678809b6d94d95c9ff6ad02238a9552363620d13b8beaa5d4b67ade9ef425c
## Issue being fixed or feature implemented
it was picking the wrong DMN as a payee...
## What was done?
see code and notes
## How Has This Been Tested?
run tests
## Breaking Changes
n/a
## Checklist:
<!--- Go over all the following points, and put an `x` in all the boxes
that apply. -->
- [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
**For repository code-owners and collaborators only**
- [x] I have assigned this pull request to a milestone
deaa6dd144f5650b385658a0c4f9a014aff8dde2 psbt: check output index is within bounds before accessing (Andrew Chow)
f1ef7f0aa46338f4cd8de79696027a1bf868f359 Don't calculate tx fees for PSBTs with invalid money values (Andrew Chow)
Pull request description:
Fixes#17149
Two classes of issues were found by the psbt fuzzer: values out of range and causing overflows, and prevout indexes being out of range. This PR fixes both.
When accessing a specific output using the index given in the tx, check that it is actually a possible output before trying to access the output.
When summing and checking amounts for `decodepsbt` and `analyzepsbt`, make sure that the values are actually valid money values.. Otherwise, stop summing and don't show the fee. For `analyzepsbt`, return that the next role is the Creator since the Creator needs to remake the transaction to be valid.
ACKs for top commit:
practicalswift:
ACK deaa6dd144f5650b385658a0c4f9a014aff8dde2 -- only change since last ACK was the addition of tests
gwillen:
tested ACK deaa6dd, would also like to see this merged!
Tree-SHA512: 06c36720bbb5a7ab1c29f7d15878bf9f0d3e5760c06bff479d412e1bf07bb3e0e9ab6cca820a4bfedaab71bfd7af813807e87cbcdf0af25cc3f66a53a06dbcfd
773d4572a4864ab7b6380858d07d9579ff6dd9a2 Mark PSBTs spending unspendable outputs as invalid in analysis (Andrew Chow)
638e40cb6080800c7b0a7f4028f63326acbe4700 Have a PSBTAnalysis state that indicates invalid PSBT (Andrew Chow)
Pull request description:
When analyzing an unspendable PSBT, report that it is unspendable and exit analysis early.
ACKs for top commit:
Sjors:
ACK 773d457
instagibbs:
After some thought ACK 773d4572a4
Tree-SHA512: 99b0cb2fa1ea37593fc65a20effe881639d69ddeeecf5197bc87bc7f2220cbeb40f1d429d517e4d27f2e9fb563a00cd845d2b4b1ce05246a75a6cb56fb9b0ba5
91cc18f602fe2ff7fe47335a8e1e7734895a19d9 [docs] Add release notes for PR 15427 (John Newbery)
3b11420b3c91f731b03805a39e48ee32e54484a2 [RPC] add new utxoupdatepsbt arguments to the CRPCCommand and CPRCConvertParam tables (John Newbery)
Pull request description:
The new `descriptors` argument was not added to the CRPCCommand and CPRCCvertParam tables, meaning that it couldn't be used with bitcoin-cli or named arguments.
Before this PR:
```
> bitcoin-cli utxoupdatepsbt 'cHNidP8BAFMCAAAAAYCdwVRx2X3o4KHx5tAMsN1ddp51MbfWsietjfMbl5HtAAAAAAD/////AQDh9QUAAAAAF6kUW+rtEOi4nk9rpw2F5XZl1dd8ehGHAAAAAAAAAA==' "[{\"desc\":\"sh(wpkh([bd50871a/0h/0h/0h]03895c66337b38699bfafff1084ad35bc347fac4f4e5e5fe5eb7dd81155280db53))\"}]"
error code: -3
error message:
Expected type array, got string
> bitcoin-cli --named utxoupdatepsbt psbt='cHNidP8BAFMCAAAAAYCdwVRx2X3o4KHx5tAMsN1ddp51MbfWsietjfMbl5HtAAAAAAD/////AQDh9QUAAAAAF6kUW+rtEOi4nk9rpw2F5XZl1dd8ehGHAAAAAAAAAA==' descriptors="[{\"desc\":\"sh(wpkh([bd50871a/0h/0h/0h]03895c66337b38699bfafff1084ad35bc347fac4f4e5e5fe5eb7dd81155280db53))\"}]"
error code: -8
error message:
Unknown named parameter descriptors
```
After this PR:
```
bitcoin-cli utxoupdatepsbt 'cHNidP8BAFMCAAAAAYCdwVRx2X3o4KHx5tAMsN1ddp51MbfWsietjfMbl5HtAAAAAAD/////AQDh9QUAAAAAF6kUW+rtEOi4nk9rpw2F5XZl1dd8ehGHAAAAAAAAAA==' "[{\"desc\":\"sh(wpkh([bd50871a/0h/0h/0h]03895c66337b38699bfafff1084ad35bc347fac4f4e5e5fe5eb7dd81155280db53))\"}]"
cHNidP8BAFMCAAAAAYCdwVRx2X3o4KHx5tAMsN1ddp51MbfWsietjfMbl5HtAAAAAAD/////AQDh9QUAAAAAF6kUW+rtEOi4nk9rpw2F5XZl1dd8ehGHAAAAAAAAAA==
bitcoin-cli --named utxoupdatepsbt psbt='cHNidP8BAFMCAAAAAYCdwVRx2X3o4KHx5tAMsN1ddp51MbfWsietjfMbl5HtAAAAAAD/////AQDh9QUAAAAAF6kUW+rtEOi4nk9rpw2F5XZl1dd8ehGHAAAAAAAAAA==' descriptors="[{\"desc\":\"sh(wpkh([bd50871a/0h/0h/0h]03895c66337b38699bfafff1084ad35bc347fac4f4e5e5fe5eb7dd81155280db53))\"}]"
cHNidP8BAFMCAAAAAYCdwVRx2X3o4KHx5tAMsN1ddp51MbfWsietjfMbl5HtAAAAAAD/////AQDh9QUAAAAAF6kUW+rtEOi4nk9rpw2F5XZl1dd8ehGHAAAAAAAAAA==
```
ACKs for top commit:
promag:
ACK 91cc18f.
fanquake:
re-ACK 91cc18f602fe2ff7fe47335a8e1e7734895a19d9
Tree-SHA512: 279b2339a5cac17e363002e4ab743e251d6757c904c89f1970575bdce18d4f63d5e13507e171bf2bdc1bf6dd457db345a4b11b15d4ff71b96c2fedc4ffe52b23
58cfbc38e040925b51cb8d35d23b50e9cf06fb2a Ignoring (but warn) on duplicate -wallet parameters (Jonas Schnelli)
Pull request description:
I expect that there are many users with load on startup wallet definitions in `bitcoin.conf` or via startup CLI argument.
With the new `settings.json` r/w configuration file, users unloading and loading a wallet through the GUI or via the RPC calls might end up with a duplicate `-wallet` entry (one that still remains in bitcoin.conf or CLI) plus the new duplication in `settings.json` due to the unload/load.
Steps to reproduce
* create wallet (if via RPC set `load_on_startup` or unloadwallet/loadwallet then set `load_on_startup`).
* stop bitcoin
* start bitcoind again with same `--wallet=mywallet`
I guess it is acceptable to skip duplicates.
ACKs for top commit:
achow101:
Tested ACK 58cfbc38e040925b51cb8d35d23b50e9cf06fb2a
meshcollider:
Code review ACK 58cfbc38e040925b51cb8d35d23b50e9cf06fb2a
ryanofsky:
Code review ACK 58cfbc38e040925b51cb8d35d23b50e9cf06fb2a. Changes since previous review: rebased, tweaked warning message, squashed/fixed test
Tree-SHA512: f94e5a999bdd7dc291f0bc142911b0a8033929350d6f6a35b58c4a06a3c8f83147be0f0c402d4e946dedbbcc85b7e023b672c731b6d7a8984d4780017c961cfb
fa5f46600fb98f1b35346bedc1a66c9019d01114 test: Fix rpc_net intermittent issue (MarcoFalke)
Pull request description:
Without the sync, the nodes might generate blocks at the same height and thus never be able to sync
ACKs for top commit:
practicalswift:
ACK fa5f46600fb98f1b35346bedc1a66c9019d01114: patch looks correct
Tree-SHA512: 21255795c2121c71fc620beb766855e57c7af94a668331d1b625665e22eb4b485a2b5c3ad2bb9a7042744f3c3e49c71251bcec41ba25bca03fd54aae32968a3a
