Follow-up to #28848
When submitpackage produced no per-transaction result for a member, the RPC set "error": "unevaluated" but then continued without inserting the entry into tx-results, making it impossible for callers to know which wtxids were unevaluated.
This inserts the error result before continuing, updates help text, and adjusts functional tests to expect entries for all submitted wtxids.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33427.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | glozow, instagibbs |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Concept ACK, this looks like an unintentional drop. I thought “unknown-not-validated” would be better since it matches the error string for skipped transactions, but I think it’s better to have separate messages for “nothing was validated because the package was ill-formed” vs “this particular transaction wasn’t validated.”
cc @instagibbs
I thought “unknown-not-validated” would be better since it matches the error string for skipped transactions, but I think it’s better to have separate messages for “nothing was validated because the package was ill-formed” vs “this particular transaction wasn’t validated.”
Right now the code can only ever return the former, as both the RPC code and the actual acceptance code are all-or-nothing. I added a check for the latter in case it ever changes, as the API allows for it.
1086 auto it = package_result.m_tx_results.find(tx->GetWitnessHash());
1087 if (it == package_result.m_tx_results.end()) {
1088- // No results, report error and continue
1089- result_inner.pushKV("error", "unevaluated");
1090+ // No per-tx result for this wtxid
1091+ result_inner.pushKV("error", any_per_tx_results ? "unknown-not-validated" : "package-not-validated");
Currently not reachable as the RPC:
CHECK_NONFATAL(m_tx_results.size() == txns.size() || m_tx_results.empty())
and acceptance code:
Assume(results_final.size() == package.size())
both are all-or-nothing in terms of package result reporting. I put that there as a reserve possibility since the API seems to allow for it.
Happy to roll back to my original PR code, though, just renamed to package-not-validated.
When submitpackage produced no per-transaction result for a member,
the RPC previously set "error": "unevaluated" but then continued
without inserting the entry into tx-results, making it impossible for
callers to know which wtxids were unevaluated.
Insert the placeholder result before continuing, update help text, and
adjust functional tests to expect entries for all submitted wtxids.
1088- // No results, report error and continue
1089- result_inner.pushKV("error", "unevaluated");
1090+ // No per-tx result for this wtxid
1091+ // Current invariant: per-tx results are all-or-none (every member or empty on package abort).
1092+ // If any exist yet this one is missing, it's an unexpected partial map.
1093+ CHECK_NONFATAL(package_result.m_tx_results.empty());
Do we want the RPC to return an error, or stumble through and try to return something?
Middle ground is adding an Assume() that gets compiled out in release builds
@glozow
Assume might make it seem like this API quirk is more important than it actually is, and then we’d have to explain why it’s ok to delete it later.
This is somewhat academic today as the RPC already errors on anything but all-or-none results from ProcessNewPackage:
CHECK_NONFATAL(m_tx_results.size() == txns.size() || m_tx_results.empty())
So if partial maps from ProcessNewPackage were ever done in practice, a call to submitpackage would run into that first check. If that guard were relaxed and tested to accommodate the change, the new check here would (appropriately) error as belt-and-suspenders, alerting the developer to change that as well. My feeling is that we should use the same check (CHECK_NONFATAL or Assume) for both places.
I prefer that the RPC method guarantees a result for each wtxid, so having guards to keep it so makes sense to me.
967@@ -968,7 +968,7 @@ static RPCHelpMan submitpackage()
968 {{RPCResult::Type::STR_HEX, "", "transaction wtxid in hex"},
969 }},
970 }},
971- {RPCResult::Type::STR, "error", /*optional=*/true, "The transaction error string, if it was rejected by the mempool"},
972+ {RPCResult::Type::STR, "error", /*optional=*/true, "Error string if rejected from mempool, or \"package-not-validated\" when the package aborts before any per-tx processing."},
I’d think it stays optional, as successful (and partially successful) submissions won’t have an error for every tx. See, eg, this test, where only the child has an error.
Edit: this contrasts with the fact that we still got a result (ie - a tx-results entry) for every tx.
1081@@ -1082,10 +1082,15 @@ static RPCHelpMan submitpackage()
1082 for (const auto& tx : txns) {
1083 UniValue result_inner{UniValue::VOBJ};
1084 result_inner.pushKV("txid", tx->GetHash().GetHex());
1085+ const auto wtxid_hex = tx->GetWitnessHash().GetHex();
Fwiw not a huge deal but I wouldn’t recommend doing an unnecessary refactor at the same time as a fix to the missing RPC result.
Looks like this is creating a copy of the string to avoid 2 calls to GetHex(), which seems sensible.