rpc/wallet: add simulaterawtransaction RPC #22751

test/functional/wallet_analyzerawtx.py:60:9: F841 local variable 'analysis' is assigned to but never used

Thanks, fixed.

<details><summary>Some suggestions for the test</summary><p>

--- 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()

</p></details>

<details><summary>Some suggestions for the rpc</summary><p>

--- 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,          },

</p></details>

• 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

Thanks, addressed all. See also alt PR.

Nit: Consider please:

    if (!DecodeHexTx(mtx, request.params[0].get_str(), /* try_no_witness */ true, /* try_witness */ true)) {

Nit: People typically place consts before types in this codebase, AFAIK. If I'm wrong, please correct me.

Looking around in the code, the way I wrote it seems to be the way it's written elsewhere, so keeping as is.

Two lines above the const modifier is before type. Anyway, it's not important so I don't want to waste your time on this any further.

That would actually make the shared pointer a constant, which points to a non-constant CWallet instance.

See e.g. https://stackoverflow.com/questions/7526152/easy-rule-to-read-complicated-const-declarations

Also try modifying my PR to this, and try compiling:

    const std::shared_ptr<CWallet> wallet = GetWalletForJSONRPCRequest(request);
    wallet->ResendWalletTransactions(); // <- this is not const

(it works, even though ResendWalletTransactions() is NOT const-declared)

Actually, I'm mistaken, and the two versions are identical. (ResendWalletTransactions() compiles for both cases.) I actually think this is a mistake, and could just as well be

std::shared_ptr<CWallet> wallet = GetWalletForJSONRPCRequest(request);

but I'm just going to keep the const where it is, as that's the case for other RPC calls. Sorry for the misinformation!

To clarify, I mis-remembered const as applying to the thing directly to the right of it, but it's the opposite; const as the starting keyword is simply a remnant from before the const rule was solidified, and is an exception that says "usually we apply to the token to our left, but when const is preceded by nothing at all, it applies to the thing right after it". (That stack overflow post explains it.)

So

const std::shared_ptr<CWallet> wallet = GetWalletForJSONRPCRequest(request);
std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);

all lead to a wallet whose non-const methods can be called, and the const applies to the std::shared_ptr in both cases (it stops us from changing wallet to, say, some other CWallet somewhere).

What we really want here is actually

std::shared_ptr<CWallet const> const wallet = GetWalletForJSOMRPCRequest(request);

I opened a separate pull request on this subject. Thanks for the nudge. https://github.com/bitcoin/bitcoin/pull/22787

Would it be more idiomatic to use for (CTxIn& txin : mtx.vin) {?

Done.

    RPCTypeCheck(request.params, {UniValue::VSTR}, /* fAllowNull */ true);

If you allow null, then L4598 will lead to a crash, would it not?

The RPCHelpMan framework will not execute the code, since the first param is non-optional.

Would RPCTypeCheck(request.params, {UniValue::VSTR}) work too then?

Would it be better to use Decimal here?

Could, but would be another import just for this one place, and the code should be fine (i.e. no rounding errors) due to the assert_approx.

In 4d6c1a01532fa189c44714fae31916d758f3a574 "rpc/wallet: add simulaterawtransaction RPC"

The isminetype here needs to match the isminetype below as otherwise we could get incorrect balance calculations. This either needs to be changed to ISMINE_ALL or the calculation in the outputs needs to check ISMINE_SPENDABLE too.

I think it would be better to use ISMINE_ALL so that the balance computation includes watchonly things as those are part of the wallet balance as well.

Or there could be an include_watchonly option that would change the IsMine filter from ISMINE_SPENDABLE to ISMINE_ALL. This would be the most inline with the rest of the RPCs.

Sounds good - adding an include_watchonly option.

In 4d6c1a01532fa189c44714fae31916d758f3a574 "rpc/wallet: add simulaterawtransaction RPC"

I would prefer using one node with 2 wallets. Having multiple nodes, with one wallet on each, is how we used to do things before multiwallet. Given that the test already makes testing wallets (one on each node), it's unnecessary to have multiple nodes.

That is obviously the right way to do it. Thanks, fixed.

In 40944a573a21c225662ce0c8ba7f64a8427ca22d "rpc/wallet: add simulaterawtransaction RPC"

This is unnecessary since no key access is needed.

Do the vin/vout checks not access keys to determine if the wallet owns them?

It only uses pubkeys.

OK, you're talking about the EnsureWalletIsUnlocked line. I thought you were also talking about L4698. Fixed.

Needs to use self.gener... after a rebase

Thanks, updated.

Would be better to use Decimal instead of float for monetary amounts.

importpubkey does not exist for descriptor wallets (sqlite).

test_framework.authproxy.JSONRPCException: Cannot import descriptor without private keys to a wallet with private keys enabled (-4)

Maybe I should rethink the usage of importpubkey completely. It seems to be not very well supported anymore.

In ae8564c198a3f9c57cc896489afcb78d6b947b45 "rpc/wallet: add simulaterawtransaction RPC"

The wallet does not need to be unlocked, so this help text is not needed.

Good point, removed.

In ae8564c198a3f9c57cc896489afcb78d6b947b45 "rpc/wallet: add simulaterawtransaction RPC"

Since tx1 is already sent, it will include inputs that are already spent. It seems like we should disallow transactions that spend already spent inputs, at least until we implement conflict checking in this RPC.

Added fetch/check of coins for transactions and RPC asserts when coins are missing or spent.

In aaa078857aa0bd4d881f781992dfc7c11aef0451 "rpc/wallet: add simulaterawtransaction RPC"

I think this section isn't doing anything now.

Re-added tests for tx2 only.

In aaa078857aa0bd4d881f781992dfc7c11aef0451 "rpc/wallet: add simulaterawtransaction RPC"

If the UTXO points to one of the transactions in the array, then this would fail. I think it is possible someone would do this, especially with package relay on the horizon.

We could have coins outside of the txs loop, add the outputs of each tx to coins, and also remove coins as they are processed in this loop.

To avoid things like tx1 spending tx2 and tx2 spending tx1, I think we may want to strictly require that transactions are in order. If we do, I think we can just iterate one at a time and just append new outputs as we go, without the extra round.

I updated the code to reflect this suggestion.

Edit：re-reading your suggestion, I think maybe you suggested the same thing I am saying..

In 84a745eed211b597a89f3bbc3da73b0ac2cb457d "rpc/wallet: add simulaterawtransaction RPC"

This loop doesn't make sense here. It shouldn't be necessary to iterate txs at the same time we are already iterating txs.

At best, this could be moved outside of the first txs loop as a preprocessing step to extract all of the UTXOs created, but I don't think that would be a good idea. That would allow for transactions that have circular dependencies and those would be invalid. It is fine to have an order requirement as that is what package validation requires.

Yeah, this was clearly a mistake. I removed the loop and kept the inner block only.

In 84a745eed211b597a89f3bbc3da73b0ac2cb457d "rpc/wallet: add simulaterawtransaction RPC"

This can be moved into the loop below that iterates the outputs.

Moved.

In 84a745eed211b597a89f3bbc3da73b0ac2cb457d "rpc/wallet: add simulaterawtransaction RPC"

Wallet functional tests should appear in test_runner.py twice, once with --legacy-wallet and once with --descriptors.

Got it. Done.

I hope this line isn't overly confusing.

This results in Decimal('0.000010000000000000000818030539140313095458623138256371021270751953125')

Better to just insist v and vexp have compatible types, and default vspan sanely?

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?

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).

I'm not sure what you're proposing. Mind writing a patch that describes your suggestion?

sure, it's merely about removing the coins fetching from chain and using the wallet information to answer the spendability question.

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.

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. :)

Shouldn't these 4 cases raise the exception One or more transaction inputs are missing ?

Anyway, this exception is not being tested.

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.

nit: perhaps the op variable can be renamed to outpoint for the sake of clarity.

Sure

ced8a3e outpoint isn't mutated

            const auto& outpoint = txin.prevout;

ced8a3e IIRC GetHash() is uint256, which is worth not copying (unsure if compilers do automatic copy elision here)

        const uint256 hash{mtx.GetHash()};

or

        const uint256& hash = mtx.GetHash();

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.

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.

https://github.com/bitcoin/bitcoin/commit/ced8a3e77d0dde428360f2250576582b624c013c

                    {"include_watchonly", RPCArg::Type::BOOL, RPCArg::DefaultHint{"true for watch-only wallets, otherwise false"}, "Whether to include watch-only addresses (see RPC importaddress)"},

https://github.com/bitcoin/bitcoin/commit/ced8a3e77d0dde428360f2250576582b624c013c my understanding was that a reference is preferred? (see RPCHelpMan getbalances for an example)

    const CWallet& wallet = *wallet;

Pretty sure this is not optional (should be NO, instead of OMITTED_NAMED_ARG)

Also, the newline seems spurious?

style nit: \n are not needed, as they don't change the help output

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"

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.

What is split?

Why is this function needed in the first place?

No need to skip the sync_fun. With one node, it will be a no-op anyway

Same

Can be removed