| Age | Commit message (Collapse) | Author |
|---|
|
RPC_TYPE_ERROR, not RPC_MISC_ERROR
e68d380797918e655decb76fc11725197d6d5323 rpc: remove unneeded RPCTypeCheckArgument checks (furszy)
55566630c60d23993a52ed54c95e7891f4588d57 rpc: treat univalue type check error as RPC_TYPE_ERROR, not RPC_MISC_ERROR (furszy)
Pull request description:
Same rationale as #26039, tackling another angle of the problem.
#### Context
We have the same univalue type error checking code spread/duplicated few times:
`RPCTypeCheckObj`, `RPCTypeCheckArgument`, `UniValue::checkType`.
In the first two functions, we are properly returning an `RPC_TYPE_ERROR` while in `UniValue::checkType`
we are throwing an `std::runtime_error` which is caught by the RPC server request handler, who invalidly
treats it as `RPC_MISC_ERROR` (which is a generic error return code that provides no information to the user).
#### Proposed Changes
Throw a custom exception from `Univalue::checkType` (instead of a plain
`std::runtime_error`) and catch it on the RPC server request handler.
So we properly return `RPC_TYPE_ERROR` (-3) on every arg type error and
not the general `RPC_MISC_ERROR` (-1).
This will allow us to remove all the `RPCTypeCheckArgument` calls. As them are redundant since #25629.
Top commit has no ACKs.
Tree-SHA512: 4e4c41851fd4e2b01a2d8b94e71513f9831f810768ebd89684caca4901e87d3677980003949bcce441f9ca607a1b38a5894839b6c492f5947b8bab8cd9423ba6
|
|
By throwing a custom exception from `Univalue::checkType` (instead of a plain
std::runtime_error) and catching it on the RPC server request handler.
So we properly return RPC_TYPE_ERROR (-3) on arg type errors and
not the general RPC_MISC_ERROR (-1).
|
|
Covered cases for send() and walletcreatefundedpsbt() RPC commands:
1. Default add_inputs value with no preset inputs (add_inputs=true):
Expect: automatically add coins from the wallet to the tx.
2. Default add_inputs value with preset inputs (add_inputs=false):
Expect: disallow automatic coin selection.
3. Explicit add_inputs=true and preset inputs (with preset inputs not-covering the target amount).
Expect: include inputs from the wallet.
4. Explicit add_inputs=true and preset inputs (with preset inputs covering the target amount).
Expect: only preset inputs are used.
5. Explicit add_inputs=true, no preset inputs (same as (1) but with an explicit set):
Expect: include inputs from the wallet.
|
|
|
|
|
|
If an external input's utxo was created by a transaction that the wallet
knows about, then it would not be selected using SelectExternal. This
results in either funding failure or incorrect weight calculation.
|
|
|
|
|
|
Added tests to rpc_fundrawtransaction, wallet_send, and rpc_psbt that
test that external inputs can be spent when input weight is provided.
Also tested that the input weight overrides any calculated weight.
Additionally, rpc_psbt's external inputs test is cleaned up a bit to be
more similar to rpc_fundrawtransaction's and avoid potential pitfalls
due to non-deterministic coin selection behavior.
|
|
|
|
getmempool RPCs
2f9515f37addabde84c79926d7a24b2897a21dd1 rpc: move fees object to match help (josibake)
07ade7db8f919826c5e69bdaf7d54a6ae653175e doc: add release note for fee field deprecation (josibake)
2ee406ce3e9c252734cb391d85044ac389c34279 test: add functional test for deprecatedrpc=fees (josibake)
35d928c63237e31c99215e2d9d84782befd618d5 rpc: deprecate fee fields from mempool entries (josibake)
Pull request description:
per #22682 , top level fee fields for mempool entries have been deprecated since 0.17 but are still returned. this PR properly deprecates them so that they are no longer returned unless `-deprecatedrpc=fees` is passed.
the first commit takes care of deprecation and also updates `test/functional/mempool_packages.py` to only use the `fees` object. the second commit adds a new functional test for `-deprecatedrpc=fees`
closes #22682
## questions for the reviewer
* `-deprecatedrpc=fees` made the most sense to me, but happy to change if there is a name that makes more sense
* #22682 seems to indicate that after some period of time, the fields will be removed all together. if we have a rough idea of when this will be, i can add a `TODO: fully remove in vXX` comment to `entryToJSON`
## testing
to get started on testing, compile, run the tests, and start your node with the deprecated rpcs flag:
```bash
./src/bitcoind -daemon -deprecatedrpc=fees
```
you should see entries with the deprecated fields like so:
```json
{
"<txid>": {
"fees": {
"base": 0.00000671,
"modified": 0.00000671,
"ancestor": 0.00000671,
"descendant": 0.00000671
},
"fee": 0.00000671,
"modifiedfee": 0.00000671,
"descendantfees": 671,
"ancestorfees": 671,
"vsize": 144,
"weight": 573,
...
},
```
you can also check `getmempoolentry` using any of the txid's from the output above.
next start the node without the deprecated flag, repeat the commands from above and verify that the deprecated fields are no longer present at the top level, but present in the "fees" object
ACKs for top commit:
jnewbery:
reACK 2f9515f37addabde84c79926d7a24b2897a21dd1
glozow:
utACK 2f9515f37addabde84c79926d7a24b2897a21dd1
Tree-SHA512: b175f4d39d26d96dc5bae26717d3ccfa5842d98ab402065880bfdcf4921b14ca692a8919fe4e9969acbb5c4d6e6d07dd6462a7e0a0a7342556279b381e1a004e
|
|
|
|
The previous diff touched most files in ./test/, so bump the headers to
avoid having to touch them again for a bump later.
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./test/
-END VERIFY SCRIPT-
|
|
The sync calls are redundant after a call to generate, because generate
already syncs itself.
-BEGIN VERIFY SCRIPT-
perl -0777 -pi -e 's/(generate[^\n]*\)[^\n]*)(\n|\s)+self.sync_(all|blocks)\([^\)]*\)\n/\1\n/g' $(git grep -l generate ./test)
-END VERIFY SCRIPT-
|
|
opted out
facc352648e4fe1ed9b406400b6e4a9d51f30349 test: Implicitly sync after generate*, unless opted out (MarcoFalke)
Pull request description:
The most frequent failure in functional tests are intermittent races. Fixing such bugs is cumbersome because it involves:
* Noticing the failure
* Fetching and reading the log to determine the test case that failed
* Adding a `self.sync_all()` where it was forgotten
* Spamming out a pr and waiting for review, which is already sparse
Also, writing a linter to catch those is not possible, nor is review effective in finding these bugs prior to merge.
Fix all future intermittent races caused by a missing sync_block call by calling `sync_all` implicitly after each `generate*`, unless opted out. This ensures that the code is race-free (with regards to blocks) when the tests pass once, instead of our current approach where the code can never be guaranteed to be race-free.
There are some scripted-diff cleanups (see https://github.com/bitcoin/bitcoin/pull/22567), but they will be submitted in a follow-up to reduce the conflicts in this pull.
ACKs for top commit:
lsilva01:
tACK facc352 on Ubuntu 20.04
brunoerg:
tACK facc352648e4fe1ed9b406400b6e4a9d51f30349 on MacOS 11.6
Tree-SHA512: 046a40a066b4a3bd28a3077bd654fa8887442dd1f0ec6fd11671865809ef02376f126eb667a1320ebd67b6e372c78c00dbf8bd25d86ed86f1d9a25363103ed97
|
|
Unless `-deprecatedrpc=fees` is passed, top level
fee fields are no longer returned for mempool entries.
Add instructions to field help on how to access
deprecated fields, update help text for readability,
and include units. This is important to help
avoid any confusion as users move from deprecated
fields to the fee fields object (credit: jonatack).
This affects `getmempoolentry`, `getrawmempool`,
`getmempoolancestors`, and `getmempooldescendants`
Modify `test/functional/mempool_packages.py` and
`test/functional/rpc_fundrawtransaction.py` tests
to no longer use deprecated fields.
Co-authored-by: jonatack <jon@atack.com>
|
|
|
|
When calculating a txs absolute fee, if the fee is rounded down to the
nearest satoshi, it is possible for the coin selection algorithms to
undercalculate the fee needed. This can lead to an assertion error in
some situations. One such scenario is added to
rpc_fundrawtransaction.py.
|
|
|
|
|
|
|
|
Don't assume that specific inputs are going to be used when they aren't
specified explicitly.
Also fixes a bug in the include_unsafe test where after the inputs
confirm, include_unsafe should be set to False rather than True.
|
|
For the test that checks that there is no error when change is
unavailable but change is also not needed, use specific UTXOs so that
SRD does not cause this to fail when it chooses random inputs.
|
|
For some of the tests within rpc_fundrawtx, there is the expectation
that two independent calls to coin selection RPCs will use the same type
of UTXO. This is not necessarily guaranteed, so to make sure it is, use
lockunspent prior to those tests.
|
|
-BEGIN VERIFY SCRIPT-
sed --regexp-extended -i \
's/((self\.)?(nodes\[[^]]+\]|[a-z_]*(wallet|node)[0-9a-z_]*))\.(generate(|toaddress|block|todescriptor)(\(|, ))/self.\5\1, /g' \
$(git grep -l generate ./test | grep -v 'test_framework/' | grep -v 'feature_rbf')
-END VERIFY SCRIPT-
|
|
Instead of always choosing BnB if it finds a solution, always do both
BnB and KnapsackSolver and choose the one which has the least waste.
|
|
functional tests for efficiency
47c48b5f35b4910fcf87caa6e37407e67d879c80 test: only use verbose for getrawmempool when necessary in functional tests (Michael Dietz)
77349713b189e80f2c140db4df50177353a1cb83 test: use getmempoolentry instead of getrawmempool in functional tests when appropriate (Michael Dietz)
86dbd54ae8a8f9c693c0ea67114bbff24a0754df test: improve mempool_updatefrom efficiency by using getmempoolentry for specific txns (Michael Dietz)
Pull request description:
I don't think this changes the intention of the test. But it does shave ~30 seconds off the time it takes to run. From what I've seen our CI `macOS 11 native [gui] [no depends]` runs `mempool_updatefrom.py` in ~135 seconds. After this PR it should run in ~105 seconds
I noticed this improvement should probably be made when testing performance/runtimes of https://github.com/bitcoin/bitcoin/pull/22698. But I wanted to separate this out from that PR so the affects of each is decoupled
Edit: The major change in this PR is improving mempool_updatefrom.py's runtime as this is a very long running test. Then made the same efficiency improvements across all the functional tests as it made since to do that here
ACKs for top commit:
theStack:
Tested ACK 47c48b5f35b4910fcf87caa6e37407e67d879c80
Tree-SHA512: 40f553715f3d4649dc18c2738554eafaca9ea800c4b028c099217896cc1c466ff457ae814d59cf8564c782a8964d8fac3eda60c1b6ffb08bbee1439b2d34434b
|
|
|
|
appropriate
|
|
ApproximateBestSubset had an edge case (due to not using
GetSelectionAmount) where it was possible for it to return success but
fail to select enough to cover transaction fees. A test is added that
could trigger this failure prior to the fix being implemented.
|
|
Adds an error output parameter to all GetReservedDestination functions
so that callers can get the actual reason that a change address could
not be fetched. This more closely matches GetNewDestination. This allows
for more granular error messages, such as one that indicates that
bech32m addresses cannot be generated yet.
|
|
11d6459b6e101f05f36e13799c400bef82d2fc21 rpc: include_unsafe option for fundrawtransaction (t-bast)
Pull request description:
Allow RPC users to opt-in to unsafe inputs when funding a raw transaction.
Applications that need to manage a complex RBF flow (such as lightning nodes using anchor outputs) are very limited if they can only use safe inputs.
I also added this option to `send` and `walletcreatefundedpsbt` who internally delegate to `fundrawtransaction`.
Fixes #21299
ACKs for top commit:
laanwj:
Code review ACK 11d6459b6e101f05f36e13799c400bef82d2fc21
Tree-SHA512: 5e542a4febcfd6f41cf784678ff02ec9282eae2082c274983f72c5ea87b7ebbe1bd5fdc6a020d7a9d5996157754eb4966b8aeb6c1ceebf0b1519f735579b8bac
|
|
|
|
|
|
|
|
|
|
Allow RPC users to opt-in to unsafe inputs when funding a raw transaction.
Applications that need to manage a complex RBF flow (such as lightning
nodes using anchor outputs) are very limited if they can only use safe inputs.
Fixes #21299
|
|
|
|
1) add a new sane "address" field (for outputs that have an
identifiable address, which doesn't include bare multisig)
2) with -deprecatedrpc: leave "reqSigs" and "addresses" intact
(with all weird/wrong behavior they have now)
3) without -deprecatedrpc: drop "reqSigs" and "addresses" entirely,
always.
|
|
|
|
will end up being too large after signing
48a0319babb409cf486a9eb7c776810f70b06cb2 Add a test that selects too large if BnB is used (Andrew Chow)
3e69939b78d0143d514c5d9b6c6a9844c9bb901c Fail if maximum weight is too large (Andrew Chow)
51e2cd322cfc7271af309e3a2243448a2ec0cad4 Have CalculateMaximumSignedTxSize also compute tx weight (Andrew Chow)
Pull request description:
Currently the `Transaction too large` is calculated on the transaction that is returned from `CreateTransaction`. This does not make sense for when `CreateTransaction` is being used for `fundrawtransaction` as no signing occurs so the final returned transaction is missing signatures. Thus users may successfully fund a transaction but fail to broadcast it after it has been fully signed.
So instead we should figure out whether the transaction we are funding will be too large after it is signed. We can do this by having `CalculateMaximumSignedTxSize` also return the transaction weight and then comparing that weight against the maximum weight.
ACKs for top commit:
instagibbs:
ACK https://github.com/bitcoin/bitcoin/pull/20536/commits/48a0319babb409cf486a9eb7c776810f70b06cb2
meshcollider:
utACK 48a0319babb409cf486a9eb7c776810f70b06cb2
Xekyo:
utACK with nits 48a0319babb409cf486a9eb7c776810f70b06cb2
Tree-SHA512: 1700c60b07f67e2d5c591c5ccd131ac9f1861fab3def961c3c9c4b3281ec1063fe8e4f0f7f1038cac72692340856406bcee8fb45c8104d2ad34357a0ec878ac7
|
|
If BnB is used, the test will fail because the transaction is too large.
|
|
|
|
A check to raise an error on zero-fee txns was mistakenly extended in commit
a0d4957 from the bumpfee and send{toaddress, many} RPCs to also include
fundrawtransaction and walletcreatefundedpsbt.
This commit 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.
|
|
in RPCs fundrawtransaction and walletcreatefundedpsbt only.
This provides the existing feeRate (BTC/kvB) behavior in these two RPCs to the
new fee_rate (sat/vB) param also.
See these two GitHub review discussions for more info:
https://github.com/bitcoin/bitcoin/pull/10706/#discussion_r126560525
https://github.com/bitcoin/bitcoin/pull/20305#discussion_r520032533
|
|
Co-authored-by: Murch <murch@murch.one>
|
|
and BTC/kvB for feeRate error messages.
|
|
Create a fee_rate (sat/vB) RPC param and replace overloading the conf_target and
estimate_mode params in the following 6 RPCs with it:
- sendtoaddress
- sendmany
- send
- fundrawtransaction
- walletcreatefundedpsbt
- bumpfee
In RPC bumpfee, the previously existing fee_rate remains but the unit is changed
from BTC/kvB to sat/vB. This is a breaking change, but it should not be an
overly risky one, as the units change by a factor of 1e5 and any fees specified
in BTC/kvB after this commit will either be too low and raise an error or be 1
sat/vB and can be RBFed.
Update the test coverage for each RPC.
Co-authored-by: Murch <murch@murch.one>
|
|
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
|
|
Import things into a separate watchonly wallet to work with descriptor
wallets.
|
