-
Notifications
You must be signed in to change notification settings - Fork 37.7k
rpc/wallet: add simulaterawtransaction RPC #22751
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rpc/wallet: add simulaterawtransaction RPC #22751
Conversation
f7a2c20
to
70620ac
Compare
5596899
to
f1cc9a1
Compare
Concept ACK, but I don't think the RPC name represents the functionality. Analyze implies some sort of in-depth breakdown of information. Perhaps |
Agree. We already have RPC with similar name:
Or maybe add it in the results of any existing RPC |
I have no strong feelings about keeping Edit: adding to results of existing RPC sounds good to me too, but not sure which that would be. |
Can we add this to
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK. Maybe add to decoderawtransaction? (Ignore me if that's dumb, and feel free to ignore the more detailed review comments below until the direction is set.)
node1.createwallet(wallet_name='w1') | ||
w1 = node1.get_wallet_rpc('w1') | ||
|
||
node0.generatetoaddress(101, w0.getnewaddress()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggestions for the test
--- a/test/functional/wallet_analyzerawtx.py
+++ b/test/functional/wallet_analyzerawtx.py
@@ -5,12 +5,14 @@
"""Test analyzerawtransaction.
"""
+from test_framework.blocktools import COINBASE_MATURITY
from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import (
assert_approx,
assert_equal,
)
+
class AnalyzeTxTest(BitcoinTestFramework):
def set_test_params(self):
self.setup_clean_chain = True
@@ -27,14 +29,14 @@ class AnalyzeTxTest(BitcoinTestFramework):
node1 = self.nodes[1]
self.connect_nodes(0, 1)
- node0.generate(1) # Leave IBD
+ node0.generate(1) # Leave IBD
node0.createwallet(wallet_name='w0')
w0 = node0.get_wallet_rpc('w0')
node1.createwallet(wallet_name='w1')
w1 = node1.get_wallet_rpc('w1')
- node0.generatetoaddress(101, w0.getnewaddress())
+ node0.generatetoaddress(nblocks=COINBASE_MATURITY + 1, address=w0.getnewaddress())
assert_equal(w0.getbalance(), 50.0)
assert_equal(w1.getbalance(), 0.0)
@@ -43,10 +45,10 @@ class AnalyzeTxTest(BitcoinTestFramework):
tx = node1.createrawtransaction([], [{address1: 5.0}])
# node0 should be unaffected
- assert_equal(w0.analyzerawtransaction(tx), 0.0)
+ assert_equal(w0.analyzerawtransaction(tx), {"balance_change": 0.0})
# node1 should see +5 balance
- assert_equal(w1.analyzerawtransaction(tx), 5.0)
+ assert_equal(w1.analyzerawtransaction(tx), {"balance_change": 5.0})
# w0 funds transaction; it should now see a decrease in (tx fee and payment), and w1 should see the same as above
funding = w0.fundrawtransaction(tx)
@@ -54,10 +56,11 @@ class AnalyzeTxTest(BitcoinTestFramework):
bitcoin_fee = float(funding["fee"])
# node0 sees fee + 5 btc decrease
- assert_approx(w0.analyzerawtransaction(tx), -(5.0 + bitcoin_fee))
+ assert_approx(w0.analyzerawtransaction(tx)["balance_change"], -(5.0 + bitcoin_fee))
# node1 sees same as before
- assert_equal(w1.analyzerawtransaction(tx), 5.0)
+ assert_equal(w1.analyzerawtransaction(tx)["balance_change"], 5.0)
+
if __name__ == '__main__':
AnalyzeTxTest().main()
src/wallet/rpcwallet.cpp
Outdated
@@ -4685,6 +4738,7 @@ static const CRPCCommand commands[] = | |||
{ "wallet", &unloadwallet, }, | |||
{ "wallet", &upgradewallet, }, | |||
{ "wallet", &walletcreatefundedpsbt, }, | |||
{ "wallet", &analyzerawtransaction, }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggestions for the rpc
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -4647,13 +4647,16 @@ static RPCHelpMan upgradewallet()
RPCHelpMan analyzerawtransaction()
{
return RPCHelpMan{"analyzerawtransaction",
- "\nCalculate the balance change resulting in the signing and broadcasting of the given transaction.\n" +
+ "\nCalculate the balance change that would result from the signing and broadcasting of the given transaction.\n" +
HELP_REQUIRING_PASSPHRASE,
{
{"hexstring", RPCArg::Type::STR, RPCArg::Optional::NO, "The transaction hex string"},
},
RPCResult{
- RPCResult::Type::NUM, "", "The wallet balance change (negative means decrease)."
+ RPCResult::Type::OBJ, "", "",
+ {
+ {RPCResult::Type::STR_AMOUNT, "balance_change", "The wallet balance change (negative means decrease)."},
+ }
},
RPCExamples{
HelpExampleCli("analyzerawtransaction", "\"myhex\"")
@@ -4673,26 +4676,31 @@ RPCHelpMan analyzerawtransaction()
// Decode the transaction
CMutableTransaction mtx;
if (!DecodeHexTx(mtx, request.params[0].get_str(), true, true)) {
- throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed. Make sure the tx has at least one input.");
+ throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Transaction hex string decoding failed. Make sure the transaction has at least one input.");
}
// Calculate changes
- CAmount changes = 0;
+ CAmount changes{0};
- // Fetch debit; we are *spending* these; if the transaction is signed and broadcast, we will lose everything in these
+ // Fetch debit; we are *spending* these; if the transaction is signed and
+ // broadcast, we will lose everything in these.
for (size_t i = 0; i < mtx.vin.size(); ++i) {
changes -= pwallet->GetDebit(mtx.vin.at(i), ISMINE_SPENDABLE);
}
- // Iterate over outputs; we are *receiving* these, if the wallet considers them "mine"; if the transaciton is signed
- // and broadcast, we will receive everything in these
+ // Iterate over outputs; we are *receiving* these, if the wallet considers
+ // them "mine"; if the transaction is signed and broadcast, we will receive
+ // everything in these.
for (size_t i = 0; i < mtx.vout.size(); ++i) {
const CTxOut& txout = mtx.vout.at(i);
if (!pwallet->IsMine(txout)) continue;
changes += txout.nValue;
}
- return ValueFromAmount(changes);
+ UniValue result(UniValue::VOBJ);
+ result.pushKV("balance_change", ValueFromAmount(changes));
+
+ return result;
}
};
}
@@ -4762,6 +4770,7 @@ static const CRPCCommand commands[] =
{ "wallet", &abandontransaction, },
{ "wallet", &abortrescan, },
{ "wallet", &addmultisigaddress, },
+ { "wallet", &analyzerawtransaction, },
{ "wallet", &backupwallet, },
{ "wallet", &bumpfee, },
{ "wallet", &psbtbumpfee, },
@@ -4816,7 +4825,6 @@ static const CRPCCommand commands[] =
{ "wallet", &unloadwallet, },
{ "wallet", &upgradewallet, },
{ "wallet", &walletcreatefundedpsbt, },
- { "wallet", &analyzerawtransaction, },
- return an amount? (edit: seems to give the same result)
- ISTM returning the result as a JSON object is preferred
s/transaciton/transaction/
line 4610- sort here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, addressed all. See also alt PR.
@prayank23
Adding to
This requires the wallet so |
(getbalances is the replacement for getbalance). |
I think it would be better to return JSON instead of numeric, that would allow adding possible other analysis in the future without breaking backwards compatibility. |
src/wallet/rpcwallet.cpp
Outdated
if (!wallet) return NullUniValue; | ||
CWallet* const pwallet = wallet.get(); | ||
|
||
RPCTypeCheck(request.params, {UniValue::VSTR}, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RPCTypeCheck(request.params, {UniValue::VSTR}, true); | |
RPCTypeCheck(request.params, {UniValue::VSTR}, /* fAllowNull */ true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you allow null
, then L4598 will lead to a crash, would it not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RPCHelpMan framework will not execute the code, since the first param is non-optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would RPCTypeCheck(request.params, {UniValue::VSTR})
work too then?
Thanks for all the feedback. I'm rewriting this as a separate pull request that adds the feature to |
f1cc9a1
to
df84f52
Compare
Opened alternative #22776 and updated this PR. |
df84f52
to
19ef3f1
Compare
c42137c
to
f19676b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK f19676b
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. |
8dd19f4
to
c3628da
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-ACK c3628da
re-ACK bd52034 |
if isinstance(v, Decimal) or isinstance(vexp, Decimal): | ||
v=Decimal(v) | ||
vexp=Decimal(vexp) | ||
vspan=Decimal(vspan) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This results in Decimal('0.000010000000000000000818030539140313095458623138256371021270751953125')
Better to just insist v
and vexp
have compatible types, and default vspan
sanely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is to let the caller not worry too much about converting stuff to/from Decimal instances. Is there an issue due to vspan
having some noise somewhere?
Github-Pull: bitcoin#22751 Rebased-From: 6513069
This command iterates over the inputs and outputs of the given transactions, and tallies up the balance change for the given wallet. This can be useful e.g. when verifying that a coin join like transaction doesn't contain unexpected inputs that the wallet will then sign for unintentionally. Github-Pull: bitcoin#22751 Rebased-From: bd52034
bd52034
to
b557f53
Compare
Ack. Tested manually. Works fine. I have also created a follow up PR which extends the functionality of this RPC. |
ACK b557f53 |
src/wallet/rpc/wallet.cpp
Outdated
// Fetch previous transactions (inputs) | ||
std::map<COutPoint, Coin> coins; | ||
for (const CTxIn& txin : mtx.vin) { | ||
coins[txin.prevout]; // Create empty map entry keyed by prevout. | ||
} | ||
pwallet->chain().findCoins(coins); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the command description, why this is needed?
Shouldn't simulaterawtransaction
only care about how the wallet behaves if a certain transaction gets relayed to the network?.
In other words, doesn't seems to be ok to fetch inputs that are not owned/watched by this wallet as them don't mean anything for the wallet.
We could replace the input spendability check that is below using wallet.IsSpent(outpoint)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you're proposing. Mind writing a patch that describes your suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, it's merely about removing the coins fetching from chain and using the wallet information to answer the spendability question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: haven't tested it but check it out:
diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp
--- a/src/wallet/rpc/wallet.cpp (revision b557f53eac39b92bfc4922729e94087556de749a)
+++ b/src/wallet/rpc/wallet.cpp (date 1659372880572)
@@ -654,13 +654,6 @@
throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Transaction hex string decoding failure.");
}
- // Fetch previous transactions (inputs)
- std::map<COutPoint, Coin> coins;
- for (const CTxIn& txin : mtx.vin) {
- coins[txin.prevout]; // Create empty map entry keyed by prevout.
- }
- pwallet->chain().findCoins(coins);
-
// Fetch debit; we are *spending* these; if the transaction is signed and
// broadcast, we will lose everything in these
for (const auto& txin : mtx.vin) {
@@ -668,19 +661,21 @@
if (spent.count(op)) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Transaction(s) are spending the same output more than once");
}
+ spent.insert(op); // mark it as spent
if (new_utxos.count(op)) {
changes -= new_utxos.at(op);
new_utxos.erase(op);
} else {
- if (!coins.count(op)) {
- throw JSONRPCError(RPC_INVALID_PARAMETER, "One or more transaction inputs are missing");
- }
- if (coins.at(op).IsSpent()) {
+ const auto* input_wtx = pwallet->GetWalletTx(txin.prevout.hash);
+ if (!input_wtx) continue;
+ if (input_wtx->tx->vout.size() >= txin.prevout.n) continue; // todo: this should throw an error
+
+ if (!(pwallet->IsMine(input_wtx->tx->vout[txin.prevout.n]) & filter)) continue;
+ if (pwallet->IsSpent(txin.prevout)) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "One or more transaction inputs have been spent already");
}
changes -= pwallet->GetDebit(txin, filter);
}
- spent.insert(op);
}
// Iterate over outputs; we are *receiving* these, if the wallet considers
As said in the comment, it's about only using the wallet information to check the transaction and not the chain information. Mainly because, in this command, we only care about the wallet state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that's an improvement. The current implementation seems to be a bit more straightforward, even if it ends up pulling in a few extra inputs. If the difference in performance is significant it might be worth it though, but I'm dubious. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK b557f53
But as mentioned below, I think a test coverage for the exception One or more transaction inputs are missing
can be added.
assert_raises_rpc_error(-8, "One or more transaction inputs have been spent already", w0.simulaterawtransaction, [tx3]) | ||
assert_raises_rpc_error(-8, "One or more transaction inputs have been spent already", w1.simulaterawtransaction, [tx3]) | ||
assert_raises_rpc_error(-8, "One or more transaction inputs have been spent already", w0.simulaterawtransaction, [tx4]) | ||
assert_raises_rpc_error(-8, "One or more transaction inputs have been spent already", w1.simulaterawtransaction, [tx4]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't these 4 cases raise the exception One or more transaction inputs are missing
?
Anyway, this exception is not being tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. I don't think that exception will ever be triggered, because coins
is always populated with all the outputs in mtx.vin
in the earlier block. I think the best approach is to simply reword this exception to say "spent or missing" and drop the other one.
b557f53
to
66cb907
Compare
66cb907
to
ced8a3e
Compare
re-ACK ced8a3e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Light ACK ced8a3e light review, rebased to current master, debug build, checked RPC help and new test
A few suggestions.
src/wallet/rpc/wallet.cpp
Outdated
// Fetch debit; we are *spending* these; if the transaction is signed and | ||
// broadcast, we will lose everything in these | ||
for (const auto& txin : mtx.vin) { | ||
auto& outpoint = txin.prevout; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ced8a3e outpoint
isn't mutated
auto& outpoint = txin.prevout; | |
const auto& outpoint = txin.prevout; |
src/wallet/rpc/wallet.cpp
Outdated
// everything in these | ||
// Also populate new_utxos in case these are spent in later transactions | ||
|
||
const auto hash = mtx.GetHash(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ced8a3e IIRC GetHash() is uint256, which is worth not copying (unsure if compilers do automatic copy elision here)
const auto hash = mtx.GetHash(); | |
const uint256 hash{mtx.GetHash()}; |
or
const auto hash = mtx.GetHash(); | |
const uint256& hash = mtx.GetHash(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that differ from just doing const auto& hash =
? I assumed it wouldn't, but you're explicitly switching to uint256
so now I'm unsure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No difference between auto
and uint256
, just highlighting the return type as auto
isn't really shorter or simpler in this case than just stating the type.
src/wallet/rpc/wallet.cpp
Outdated
}, | ||
{"options", RPCArg::Type::OBJ_USER_KEYS, RPCArg::Optional::OMITTED_NAMED_ARG, "Options", | ||
{ | ||
{"include_watchonly", RPCArg::Type::BOOL, RPCArg::DefaultHint{"true for watch-only wallets, otherwise false"}, "Whether to include watch-only addresses (see 'importaddress')"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{"include_watchonly", RPCArg::Type::BOOL, RPCArg::DefaultHint{"true for watch-only wallets, otherwise false"}, "Whether to include watch-only addresses (see 'importaddress')"}, | |
{"include_watchonly", RPCArg::Type::BOOL, RPCArg::DefaultHint{"true for watch-only wallets, otherwise false"}, "Whether to include watch-only addresses (see RPC importaddress)"}, |
src/wallet/rpc/wallet.cpp
Outdated
{ | ||
std::shared_ptr<const CWallet> const wallet = GetWalletForJSONRPCRequest(request); | ||
if (!wallet) return NullUniValue; | ||
const CWallet* pwallet = wallet.get(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ced8a3e my understanding was that a reference is preferred? (see RPCHelpMan getbalances
for an example)
const CWallet* pwallet = wallet.get(); | |
const CWallet& wallet = *wallet; |
This command iterates over the inputs and outputs of the given transactions, and tallies up the balance change for the given wallet. This can be useful e.g. when verifying that a coin join like transaction doesn't contain unexpected inputs that the wallet will then sign for unintentionally.
ced8a3e
to
db10cf8
Compare
Applied suggestions by @jonatack. Thanks for the feedback! |
ACK db10cf8 A few minor ideas if you retouch (clang-tidy named args format, a const ref, a test)
--- a/src/wallet/rpc/wallet.cpp
+++ b/src/wallet/rpc/wallet.cpp
@@ -628,12 +628,12 @@ RPCHelpMan simulaterawtransaction()
UniValue include_watchonly(UniValue::VNULL);
if (request.params[1].isObject()) {
- UniValue options = request.params[1];
+ const UniValue& options = request.params[1];
RPCTypeCheckObj(options,
{
{"include_watchonly", UniValueType(UniValue::VBOOL)},
},
- true, true);
+ /*fAllowNull=*/true, /*fStrict=*/true);
include_watchonly = options["include_watchonly"];
}
@@ -650,7 +650,7 @@ RPCHelpMan simulaterawtransaction()
for (size_t i = 0; i < txs.size(); ++i) {
CMutableTransaction mtx;
- if (!DecodeHexTx(mtx, txs[i].get_str(), /* try_no_witness */ true, /* try_witness */ true)) {
+ if (!DecodeHexTx(mtx, txs[i].get_str(), /*try_no_witness=*/true, /*try_witness=*/true)) {
throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Transaction hex string decoding failure.");
}
@@ -685,7 +685,7 @@ RPCHelpMan simulaterawtransaction()
// everything in these
// Also populate new_utxos in case these are spent in later transactions
- const auto& hash = mtx.GetHash(); // auto isn't shorter or simpler here so maybe be explicit about the type
+ const uint256& hash = mtx.GetHash();
for (size_t i = 0; i < mtx.vout.size(); ++i) {
const auto& txout = mtx.vout[i];
--- a/test/functional/wallet_simulaterawtx.py
+++ b/test/functional/wallet_simulaterawtx.py
@@ -50,6 +50,9 @@ class SimulateTxTest(BitcoinTestFramework):
tx1 = node.createrawtransaction([], [{address1: 5.0}])
tx2 = node.createrawtransaction([], [{address2: 10.0}])
+ self.log.info("Test simulaterawtransaction result if no txns provided") # drop this line if no other test logging
+ assert_equal(w0.simulaterawtransaction([]), {'balance_change': Decimal('0E-8')})
+
# w0 should be unaffected, w2 should see +5 for tx1
assert_equal(w0.simulaterawtransaction([tx1])["balance_change"], 0.0) |
re-ACK db10cf8 |
db10cf8 rpc/wallet: add simulaterawtransaction RPC (Karl-Johan Alm) 701a64f test: add support for Decimal to assert_approx (Karl-Johan Alm) Pull request description: (note: this was originally titled "add analyzerawtransaction RPC") This command iterates over the inputs and outputs of the given transactions, and tallies up the balance change for the given wallet. This can be useful e.g. when verifying that a coin join like transaction doesn't contain unexpected inputs that the wallet will then sign for unintentionally. I originally proposed this to Elements (ElementsProject/elements#1016) and it was suggested that I propose this upstream. There is an alternative bitcoin#22776 to instead add this info to `getbalances` when providing an optional transaction as argument. ACKs for top commit: jonatack: ACK db10cf8 achow101: re-ACK db10cf8 Tree-SHA512: adf222ec7dcdc068d007ae6f465dbc35b692dc7bb2db337be25340ad0c2f9c64cfab4124df23400995c700f41c83c29a2c34812121782c26063b100c7969b89d
return RPCHelpMan{"simulaterawtransaction", | ||
"\nCalculate the balance change resulting in the signing and broadcasting of the given transaction(s).\n", | ||
{ | ||
{"rawtxs", RPCArg::Type::ARR, RPCArg::Optional::OMITTED_NAMED_ARG, "An array of hex strings of raw transactions.\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure this is not optional (should be NO
, instead of OMITTED_NAMED_ARG
)
return RPCHelpMan{"simulaterawtransaction", | ||
"\nCalculate the balance change resulting in the signing and broadcasting of the given transaction(s).\n", | ||
{ | ||
{"rawtxs", RPCArg::Type::ARR, RPCArg::Optional::OMITTED_NAMED_ARG, "An array of hex strings of raw transactions.\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the newline seems spurious?
RPCHelpMan simulaterawtransaction() | ||
{ | ||
return RPCHelpMan{"simulaterawtransaction", | ||
"\nCalculate the balance change resulting in the signing and broadcasting of the given transaction(s).\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style nit: \n
are not needed, as they don't change the help output
|
||
for (size_t i = 0; i < txs.size(); ++i) { | ||
CMutableTransaction mtx; | ||
if (!DecodeHexTx(mtx, txs[i].get_str(), /* try_no_witness */ true, /* try_witness */ true)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you trying no witness? It is impossible for a valid transaction to fail witness deserialization. See also git grep "Make sure the tx has at least one input"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that the tests are relying on no-inputs txs, but I wonder if there is a use case to run this on transactions that can never be valid on the network, even if properly signed.
def skip_test_if_missing_module(self): | ||
self.skip_if_no_wallet() | ||
|
||
def setup_network(self, split=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is split
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this function needed in the first place?
def run_test(self): | ||
node = self.nodes[0] | ||
|
||
self.generate(node, 1, sync_fun=self.no_op) # Leave IBD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to skip the sync_fun. With one node, it will be a no-op anyway
|
||
# send tx1 to avoid reusing same UTXO below | ||
node.sendrawtransaction(w0.signrawtransactionwithwallet(tx1)["hex"]) | ||
self.generate(node, 1, sync_fun=self.no_op) # Confirm tx to trigger error below |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
# send tx1 to avoid reusing same UTXO below | ||
node.sendrawtransaction(w0.signrawtransactionwithwallet(tx1)["hex"]) | ||
self.generate(node, 1, sync_fun=self.no_op) # Confirm tx to trigger error below | ||
self.sync_all() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be removed
(note: this was originally titled "add analyzerawtransaction RPC")
This command iterates over the inputs and outputs of the given transactions, and tallies up the balance change for the given wallet. This can be useful e.g. when verifying that a coin join like transaction doesn't contain unexpected inputs that the wallet will then sign for unintentionally.
I originally proposed this to Elements (ElementsProject/elements#1016) and it was suggested that I propose this upstream.
There is an alternative #22776 to instead add this info to
getbalances
when providing an optional transaction as argument.