Policy: Report reason inputs are non standard #29060
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29060. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. LLM Linter (✨ experimental)Possible typos and grammar issues:
drahtbot_id_4_m |
3fc397a to
6ffb956
Compare
6ffb956 to
e032462
Compare
|
Thank you for review @stickies-v I Addressed your comments and taken suggestions. |
e032462 to
814571b
Compare
|
Thanks for reviewing @luke-jr The changes in latest push are:
|
814571b to
0a39b07
Compare
d9e71f0 to
5ef3f9b
Compare
5ef3f9b to
f4c1945
Compare
|
Rebased on master for green CI. |
f4c1945 to
101a308
Compare
101a308 to
3ef7ac6
Compare
The release note helps a lot. To err on the side of caution, it seems appropriate to include a |
Thanks for your review @tdb3 will address this comment soon. |
I attempted this, but it's a bit non trivial with some if-else branching. I also had to add a new see **untested** diffdiff --git a/src/common/messages.cpp b/src/common/messages.cpp
index 5fe3e9e4d86..3142ca07b4c 100644
--- a/src/common/messages.cpp
+++ b/src/common/messages.cpp
@@ -132,6 +132,8 @@ bilingual_str TransactionErrorString(const TransactionError err)
return Untranslated("Transaction outputs already in utxo set");
case TransactionError::MEMPOOL_REJECTED:
return Untranslated("Transaction rejected by mempool");
+ case TransactionError::MEMPOOL_REJECTED_NON_STANDARD_INPUT:
+ return Untranslated("Transaction rejected by mempool");
case TransactionError::MEMPOOL_ERROR:
return Untranslated("Mempool internal error");
case TransactionError::MAX_FEE_EXCEEDED:
diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp
index 0f45da45dbd..c35bee4ee61 100644
--- a/src/node/transaction.cpp
+++ b/src/node/transaction.cpp
@@ -25,6 +25,9 @@ static TransactionError HandleATMPError(const TxValidationState& state, std::str
if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) {
return TransactionError::MISSING_INPUTS;
}
+ if (state.GetResult() == TxValidationResult::TX_INPUTS_NOT_STANDARD) {
+ return TransactionError::MEMPOOL_REJECTED_NON_STANDARD_INPUT;
+ }
return TransactionError::MEMPOOL_REJECTED;
} else {
return TransactionError::MEMPOOL_ERROR;
diff --git a/src/node/types.h b/src/node/types.h
index 1302f1b127f..5cdb696204f 100644
--- a/src/node/types.h
+++ b/src/node/types.h
@@ -21,6 +21,7 @@ enum class TransactionError {
MISSING_INPUTS,
ALREADY_IN_UTXO_SET,
MEMPOOL_REJECTED,
+ MEMPOOL_REJECTED_NON_STANDARD_INPUT,
MEMPOOL_ERROR,
MAX_FEE_EXCEEDED,
MAX_BURN_EXCEEDED,
diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp
index d61898260bf..a9f434fa605 100644
--- a/src/rpc/mempool.cpp
+++ b/src/rpc/mempool.cpp
@@ -95,6 +95,9 @@ static RPCHelpMan sendrawtransaction()
NodeContext& node = EnsureAnyNodeContext(request.context);
const TransactionError err = BroadcastTransaction(node, tx, err_string, max_raw_tx_fee, /*relay=*/true, /*wait_callback=*/true);
if (TransactionError::OK != err) {
+ if (IsDeprecatedRPCEnabled("nonstandard-inputs-err") && err == TransactionError::MEMPOOL_REJECTED_NON_STANDARD_INPUT) {
+ throw JSONRPCTransactionError(err, "bad-txns-nonstandard-inputs");
+ }
throw JSONRPCTransactionError(err, err_string);
}
@@ -241,7 +244,11 @@ static RPCHelpMan testmempoolaccept()
if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) {
result_inner.pushKV("reject-reason", "missing-inputs");
} else {
- result_inner.pushKV("reject-reason", state.GetRejectReason());
+ if (IsDeprecatedRPCEnabled("nonstandard-inputs-err") && state.GetResult() == TxValidationResult::TX_INPUTS_NOT_STANDA
RD) {
+ result_inner.pushKV("reject-reason", "bad-txns-nonstandard-inputs");
+ } else {
+ result_inner.pushKV("reject-reason", state.GetRejectReason());
+ }
}
}
rpc_result.push_back(std::move(result_inner));
@@ -961,7 +968,6 @@ static RPCHelpMan submitpackage()
}
UniValue rpc_result{UniValue::VOBJ};
- rpc_result.pushKV("package_msg", package_msg);
UniValue tx_result_map{UniValue::VOBJ};
std::set<uint256> replaced_txids;
for (const auto& tx : txns) {
@@ -979,7 +985,11 @@ static RPCHelpMan submitpackage()
result_inner.pushKV("other-wtxid", it->second.m_other_wtxid.value().GetHex());
break;
case MempoolAcceptResult::ResultType::INVALID:
- result_inner.pushKV("error", it->second.m_state.ToString());
+ if (IsDeprecatedRPCEnabled("nonstandard-inputs-err") && it->second.m_state.GetResult() == TxValidationResult::TX_INPUTS_N
OT_STANDARD){
+ result_inner.pushKV("error", "bad-txns-nonstandard-inputs");
+ } else {
+ result_inner.pushKV("error", it->second.m_state.ToString());
+ }
break;
case MempoolAcceptResult::ResultType::VALID:
case MempoolAcceptResult::ResultType::MEMPOOL_ENTRY:
diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
index 678bac7a185..3653421dcae 100644
--- a/src/rpc/util.cpp
+++ b/src/rpc/util.cpp
@@ -400,6 +400,8 @@ RPCErrorCode RPCErrorFromTransactionError(TransactionError terr)
switch (terr) {
case TransactionError::MEMPOOL_REJECTED:
return RPC_TRANSACTION_REJECTED;
+ case TransactionError::MEMPOOL_REJECTED_NON_STANDARD_INPUT:
+ return RPC_TRANSACTION_REJECTED;
case TransactionError::ALREADY_IN_UTXO_SET:
return RPC_VERIFY_ALREADY_IN_UTXO_SET;
default: break;
diff --git a/src/test/fuzz/kitchen_sink.cpp b/src/test/fuzz/kitchen_sink.cpp
index 62b49106cdd..4df4a2c4372 100644
--- a/src/test/fuzz/kitchen_sink.cpp
+++ b/src/test/fuzz/kitchen_sink.cpp
@@ -25,6 +25,7 @@ constexpr TransactionError ALL_TRANSACTION_ERROR[] = {
TransactionError::MISSING_INPUTS,
TransactionError::ALREADY_IN_UTXO_SET,
TransactionError::MEMPOOL_REJECTED,
+ TransactionError::MEMPOOL_REJECTED_NON_STANDARD_INPUT,
TransactionError::MEMPOOL_ERROR,
TransactionError::MAX_FEE_EXCEEDED,
};However, do you think it would be better to make the prefix of the message The only clients that affected after the above suggestion are those performing an exact match. Let me know what you think about the two approaches. |
c6eca6f doc: add guidance for RPC to developer notes (tdb3) Pull request description: Adds guidance statements to the RPC interface section of the developer notes with examples of when to implement `-deprecatedrpc=`. Wanted to increase awareness of preferred RPC implementation approaches for newer contributors. This implements some of what's discussed in #29912 (comment) Opinions may differ, so please don't be shy. We want to make RPC as solid/safe as possible. Examples of discussions where guidelines/context might have added value: #30212 (comment) #29845 (comment) #30381 (review) #29954 (comment) #30410 (review) #30713 #30381 #29060 (review) ACKs for top commit: l0rinc: ACK c6eca6f fjahr: ACK c6eca6f maflcko: lgtm ACK c6eca6f jonatack: ACK c6eca6f Tree-SHA512: 01a98a8dc0eb91762b225d3278cdb4a5e380ceb7486fd096b4ad9122bed859cea8584d8996d3dce51272fdb792f4a793a1bd1c5445efeb87f0a30f9b6e59a790
02dd534 to
b487edc
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
3cde7f1 to
4d5ede0
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
This commit renames AreInputsStandard to HasNonStandardInput. HasNonStandardInput now returns valid TxValidationState if all inputs (scriptSigs) use only standard transaction forms else returns invalid TxValidationState which states why an input is not standard.
Test the various input scriptSig non standardness, and ensure HasNonStandardInput Returns the resin why the input is nonstandard. Co-authored-by: Ben Woosley <ben.woosley@gmail.com>
4d5ede0 to
cf63e46
Compare
cf63e46 to
85c6c97
Compare
|
Rebased after the merge of #32521 to pick up recent changes to A new verbose message will be returned when the BIP54 legacy sigops limit is violated:
3cde7f185255b1bc07a90e95708665e0b6d5a46d..85c6c97b609bcbcd2b2f13638d858d5a31a903 |
This PR is another attempt at #13525.
Transactions that fail
PreChecksValidation due to non-standard inputs now returns invalid validation stateTxValidationResult::TX_INPUTS_NOT_STANDARDalong with a debug error message.Previously, the debug error message for non-standard inputs do not specify why the inputs were considered non-standard.
Instead, the same error string,
bad-txns-nonstandard-inputs, used for all types of non-standard input scriptSigs.This PR updates the
AreInputsStandardto include the reason why inputs are non-standard in the debug message.This improves the
Precheckdebug message to be more descriptive.Furthermore, I have addressed all remaining comments from #13525 in this PR.