Age | Commit message (Collapse) | Author |
|
When using `sendrawtransaction` the ALREADY_IN_CHAIN error help string
may be confusing.
Rename TransactionError::ALREADY_IN_CHAIN to
TransactionError::ALREADY_IN_UTXO_SET and update the rpc help string.
Remove backwards compatibility alias as no longer required.
|
|
-BEGIN VERIFY SCRIPT-
sed -i -e 's/\s*().main\s*()/(__file__).main()/' $(git ls-files test/functional/*.py)
sed -i -e 's/def __init__(self)/def __init__(self, test_file)/' test/functional/test_framework/test_framework.py
-END VERIFY SCRIPT-
|
|
429ec1aaaaafab150f11e27fcf132a99b57c4fc7 refactor: Rename CTransaction::nVersion to version (Ava Chow)
27e70f1f5be1f536f2314cd2ea42b4f80d927fbd consensus: Store transaction nVersion as uint32_t (Ava Chow)
Pull request description:
Given that the use of a transaction's nVersion is always as an unsigned int, it doesn't make sense to store it as signed and then cast it to unsigned everywhere it is used and displayed.
Since a few alternative implementations have recently been revealed to have made an error with this signedness that would have resulted in consensus failure, I think it makes sense for us to just make this always unsigned to make it clear that the version is treated as unsigned. This would also help us avoid future potential issues with signedness of this value.
I believe that this is safe and does not actually change what transactions would or would not be considered both standard and consensus valid. Within consensus, the only use of the version in consensus is in BIP68 validation which was already casting it to uint32_t. Within policy, although it is used as a signed int for the transaction version number check, I do not think that this change would change standardness. Standard transactions are limited to the range [1, 2]. Negative numbers would have fallen under the < 1 condition, but by making it unsigned, they are still non-standard under the > 2 condition.
Unsigned and signed ints are serialized and unserialized the same way so there is no change in serialization.
ACKs for top commit:
maflcko:
ACK 429ec1aaaaafab150f11e27fcf132a99b57c4fc7 🐿
glozow:
ACK 429ec1aaaa
shaavan:
ACK 429ec1aaaaafab150f11e27fcf132a99b57c4fc7 💯
Tree-SHA512: 0bcd92a245d7d16c3665d2d4e815a4ef28207ad4a1fb46c6f0203cdafeab1b82c4e95e4bdce7805d80a4f4a46074f6542abad708e970550d38a00d759e3dcef1
|
|
* Tx decode failed
* Missing transactions
* Input not found or already spent
|
|
In order to ensure that the change of nVersion to a uint32_t in the
previous commit has no effect, rename nVersion to version in this commit
so that reviewers can easily spot if a spot was missed or if there is a
check somewhere whose semantics have changed.
|
|
Given that the use of a transaction's nVersion is always as an unsigned
int, it doesn't make sense to store it as signed and then cast it to
unsigned.
|
|
specific error messages
98570fe29bb08d7edc48011aa6b9731c6ab4ed2e test: add coverage for parsing cryptographically invalid pubkeys (Sebastian Falbesoner)
c740b154d193b91ca42f18759098d3fef6eaab05 rpc: use `HexToPubKey` helper for all legacy pubkey-parsing RPCs (Sebastian Falbesoner)
100e8a75bf5d8196c005331bd8f2ed42ada6d8d0 rpc: check and throw specific pubkey parsing errors in `HexToPubKey` (Sebastian Falbesoner)
Pull request description:
Parsing legacy public keys can fail for three reasons (in this order):
- pubkey is not in hex
- pubkey has an invalid length (not 33 or 65 bytes for compressed/uncompressed, respectively)
- pubkey is crytographically invalid, i.e. is not on curve (`CPubKey.IsFullyValid()` check)
Many RPCs currently perform these checks manually with different error messages, even though we already have a `HexToPubKey` helper. This PR puts all three checks in this helper (the length check was done on the call-sites before), adds specific error messages for each case, and consequently uses it for all RPCs that parse legacy pubkeys. This leads to deduplicated code and also to more consistent and detailed error messages for the user.
Affected RPC calls are `createmultisig`, `addmultisigaddress`, `importpubkey`, `importmulti`, `fundrawtransaction`, `walletcreatefundedpsbt`, `send` and `sendall`.
Note that the error code (-5 a.k.a. `RPC_INVALID_ADDRESS_OR_KEY`) doesn't change in any of the causes, so the changes are not breaking RPC API compatibility. Only the messages are more specific.
The last commits adds test coverage for the cryptographically invalid (not-on-curve) pubkey case which wasn't exercised before.
ACKs for top commit:
stratospher:
tested ACK 98570fe.
davidgumberg:
ACK https://github.com/bitcoin/bitcoin/pull/28336/commits/98570fe29bb08d7edc48011aa6b9731c6ab4ed2e
Eunovo:
Tested ACK https://github.com/bitcoin/bitcoin/pull/28336/commits/98570fe29bb08d7edc48011aa6b9731c6ab4ed2e
achow101:
ACK 98570fe29bb08d7edc48011aa6b9731c6ab4ed2e
Tree-SHA512: cfa474176e95b5b18f3a9da28fdd9e87195cd58994c1331198f2840925fff322fd323a6371feab74a1b32e4b9ea58a6dc732fa751b4cdd45402c1029af609ece
|
|
when `self.noban_tx_relay=True`, the following flag
`-whitelist=noban,in,out@127.0.0.1`is added to `extra_args`
to speed up tx relay/mempool sync.
|
|
In the helper `HexToPubKey`, check for three different causes of legacy
public key parsing errors (in this order):
- pubkey is not a hex string
- pubkey doesn't have a valid length (33 or 65 bytes) [NEW]
- pubkey is cryptographically invalid, i.e. not on curve
(`IsFullyValid` check)
and throw a specific error message for each one. Note that the error
code is identical for all of them (-5), so this doesn't break RPC API
compatibility.
The helper is currently used for the RPCs `createmultisig` and
`addmultisigaddress`. The length checks can be removed from the
call-sites and error message checks in the functional tests are adapted.
|
|
This fails on master without the previous commit.
|
|
sendrawtransaction.
'maxburnamount' sets a maximum value for outputs heuristically deemed unspendable including datacarrier scripts that begin with `OP_RETURN`.
|
|
|
|
|
|
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./
-END VERIFY SCRIPT-
Commits of previous years:
- 2021: f47dda2c58b5d8d623e0e7ff4e74bc352dfa83d7
- 2020: fa0074e2d82928016a43ca408717154a1c70a4db
- 2019: aaaaad6ac95b402fe18d019d67897ced6b316ee0
|
|
getrawtransaction
f86697163e5cdbc3bc4a65cfb7dbaa3d9eb602a9 rpc: Return fee and prevout(s) to getrawtransaction (Douglas Chimento)
Pull request description:
Add fee response in BTC to getrawtransaction #23264
### For Reviewers
* Verbose arg is now an int
* Verbose = 2 includes a `fee` field and `prevout`
* [./test/functional/rpc_rawtransaction.py](./test/functional/rpc_rawtransaction.py) contains a new test to validate fields of new verbosity 2 (not the values)
```
bitcoin-cli -chain=test getrawtransaction 9ae533f7da9be4a34997db78343a8d8d6d6186b6bba3959e56f416a5c70e7de4 2 000000000000001d442e556146d5f2841d85150c200e8d8b8a4b5005b13878f6
```
```
"in_active_chain": true,
"txid": "9ae533f7da9be4a34997db78343a8d8d6d6186b6bba3959e56f416a5c70e7de4",
"hash": "7f23e3f3a0a256ddea1d35ffd43e9afdd67cc68389ef1a804bb20c76abd6863e",
....
"vin": [
{
"txid": "23fc75d6d74f6f97e225839af69ff36a612fe04db58a4414ec4828d1749a05a0",
"vout": 0,
"scriptSig": {
"asm": "",
"hex": ""
},
"prevout": {
"generated": false,
"height": 2099486,
"value": 0.00017764,
"scriptPubKey": {
"asm": "0 7846ce1ced3253d8bd43008db2ca364cc722f5a2",
"hex": "00147846ce1ced3253d8bd43008db2ca364cc722f5a2",
"address": "tb1q0prvu88dxffa302rqzxm9j3kfnrj9adzk49mlp",
"type": "witness_v0_keyhash"
}
},
"sequence": 4294967295
},
...
"fee": 0.00000762
}
```
ACKs for top commit:
achow101:
ACK f86697163e5cdbc3bc4a65cfb7dbaa3d9eb602a9
aureleoules:
ACK f86697163e5cdbc3bc4a65cfb7dbaa3d9eb602a9
hernanmarino:
re ACK f86697163e5cdbc3bc4a65cfb7dbaa3d9eb602a9
pablomartin4btc:
re-tACK f86697163e5cdbc3bc4a65cfb7dbaa3d9eb602a9
Tree-SHA512: 591fdc285d74fa7803e04ad01c7b70bc20fac6b1369e7bd5b8e2cde9b750ea52d6c70d79225b74bef4f4bbc0fb960877778017184e146119da4a55f9593d1224
|
|
Review note: The changes are complete, because self.options.descriptors
is set to None in parse_args (test_framework.py).
A value of None implies -disablewallet, see the previous commit.
So if a call to add_wallet_options is missing, it will lead to a test
failure when the wallet is compiled in.
|
|
The bool is only used to call a public helper, which some tests already
do. So use the public helper in all tests consistently and make the
confusingly named bool private.
|
|
* Add optional fee response in BTC to getrawtransaction
* Add optional prevout(s) response to getrawtransaction showing utxos being spent
* Add getrawtransaction_verbosity functional test to validate fields
|
|
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).
|
|
We were throwing two different errors for the same problematic:
* "Expected type {expected], got {type}" --> RPCTypeCheckArgument()
* "JSON value of type {type} is not of expected type {expected}" --> UniValue::checkType()
|
|
|
|
-BEGIN VERIFY SCRIPT-
sed -i 's:BIP125_SEQUENCE_NUMBER:MAX_BIP125_RBF_SEQUENCE:g' $(git grep -l BIP125_SEQUENCE_NUMBER ./test)
-END VERIFY SCRIPT-
|
|
The from_node argument is no longer used as of commit
a55606c3bdbfdf660b093bc2a618d537ffae7f26
|
|
|
|
|
|
This test was previously run twice, once with `--legacy-wallet` and once with
`--descriptors`.
Now we run it only with `--legacy-wallet`, as all the tests has been
ported to the MiniWallet but `raw_multisig_transaction_legacy_tests`,
which can be run only with the legacy wallet.
We also decrease the number of nodes used from 4 to 3, making the test
run slightly faster.
|
|
Put signrawtransactionwithwallet_tests in rpc_signrawtransaction.py,
as the test is mainly testing the signrawtransaction RPC.
Review with `git show --color-moved=dimmed-zebra`
|
|
|
|
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-
|
|
-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-
|
|
|
|
|
|
|
|
|
|
|
|
(and make the 'string "Flase"' test clearer as requested by reviewers)
|
|
|
|
|
|
|
|
|
|
in my testing from 45-55 seconds to 15.
|
|
|
|
`FromHex` is mostly used for transactions, so we introduce a
shortcut `tx_from_hex` for `FromHex(CTransaction, hex_str)`.
|
|
|
|
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./
-END VERIFY SCRIPT-
|
|
|
|
The traditional multisig workflow doesn't work with descriptor wallets
so make these tests legacy wallet only.
|
|
-BEGIN VERIFY SCRIPT-
# max-depth=0 excludes test/functional/test_framework/...
FILES=$(git grep -l --max-depth 0 "connect_nodes" test/functional)
# Replace (dis)?connect_nodes(self.nodes[a], b) with self.(dis)?connect_nodes(a, b)
sed -i 's/\b\(dis\)\?connect_nodes(self\.nodes\[\(.*\)\]/self.\1connect_nodes(\2/g' $FILES
# Remove imports in the middle of a line
sed -i 's/\(dis\)\?connect_nodes, //g' $FILES
sed -i 's/, \(dis\)\?connect_nodes//g' $FILES
# Remove imports on a line by themselves
sed -i '/^\s*\(dis\)\?connect_nodes,\?$/d' $FILES
sed -i '/^from test_framework\.util import connect_nodes$/d' $FILES
-END VERIFY SCRIPT-
Co-authored-by: Elliott Jin <elliott.jin@gmail.com>
|
|
clients
b048b275d9711f70847afaea5450f17a0f7e673a [validation] Remove absurdfee from accepttomempool (John Newbery)
932564b9cfda8446a957649c2316a52e868ad5d4 scripted-diff: update max-fee-exceeded error message to include RPC (gzhao408)
8f1290c60159a3171c27250bc95687548c5c1b84 [rpc/node] check for high fee before ATMP in clients (gzhao408)
Pull request description:
Picked up from #15810. Add separate fee-checking logic for clients that need to enforce max fee rates, then remove the `absurdFee` logic from ATMP.
ATMP's `nAbsurdFee` argument is used to enforce user-specific behavior (it is not policy since it isn't applied consistently: it is only ever used in RPC and wallet, and set to 0 everywhere else internally). It should be removed from `AcceptToMemoryPool` because (1) validation results/mempool behavior should not be user-specific and (2) enforcing a max fee rate should be the responsibility of the client instead of the mempool.
Note: this PR does not intend to _remove_ protection from high fees, just re-delegate the responsibility to clients.
ACKs for top commit:
jnewbery:
utACK b048b275d9711f70847afaea5450f17a0f7e673a
LarryRuane:
re-ACK b048b275d9711f70847afaea5450f17a0f7e673a
MarcoFalke:
re-ACK b048b275d9 , only change is squashing one commit 🏦
instagibbs:
utACK https://github.com/bitcoin/bitcoin/pull/19339/commits/b048b275d9711f70847afaea5450f17a0f7e673a
Tree-SHA512: 57c17ba16d230a4cae2896dd6a64c924f307757824e35784bf96da7b10aff2d8ea910710edf35e981035623a155f8766209a92a0fdb856549fde78bc3eaae4d2
|