Skip to content
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

Merged
merged 2 commits into from Aug 5, 2022

Conversation

kallewoof
Copy link
Member

@kallewoof kallewoof commented Aug 20, 2021

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

@kallewoof kallewoof force-pushed the 202108-analyzerawtransaction branch 2 times, most recently from f7a2c20 to 70620ac Compare August 20, 2021 06:14
@kallewoof kallewoof force-pushed the 202108-analyzerawtransaction branch 2 times, most recently from 5596899 to f1cc9a1 Compare August 20, 2021 07:13
@meshcollider
Copy link
Contributor

Concept ACK, but I don't think the RPC name represents the functionality. Analyze implies some sort of in-depth breakdown of information. Perhaps nettransactionbalance or something to represent the balance-specific use?

@ghost
Copy link

ghost commented Aug 21, 2021

but I don't think the RPC name represents the functionality. Analyze implies some sort of in-depth breakdown of information.

Agree. We already have RPC with similar name: analyzepsbt which returns lot of things that would be helpful for someone using PSBT. Although I am still confused between decodepsbt and analyzepsbt.

Perhaps nettransactionbalance or something to represent the balance-specific use?

Or maybe add it in the results of any existing RPC

@kallewoof
Copy link
Member Author

kallewoof commented Aug 21, 2021

I have no strong feelings about keeping analyze but I think <verb>rawtransaction is a good name (edit: because it is sticking to the convention), where <verb> somehow indicates looking at each input and output and determining whether they belong to the wallet, and if so to add/subtract the appropriate amount.

Edit: adding to results of existing RPC sounds good to me too, but not sure which that would be.

@ghost
Copy link

ghost commented Aug 21, 2021

adding to results of existing RPC sounds good to me too, but not sure which that would be.

Can we add this to getbalance? Will need one argument include_tx so the command would look like this:

$ bitcoin-cli -named getbalance include_tx=hex

Result:
{                   
  "balance" : n    (numeric) The total amount in BTC received for this wallet.

  "change"  : n    (numeric) The wallet balance change including tx(negative means decrease).
}

Copy link
Contributor

@jonatack jonatack left a 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())
Copy link
Contributor

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

@@ -4685,6 +4738,7 @@ static const CRPCCommand commands[] =
{ "wallet", &unloadwallet, },
{ "wallet", &upgradewallet, },
{ "wallet", &walletcreatefundedpsbt, },
{ "wallet", &analyzerawtransaction, },
Copy link
Contributor

@jonatack jonatack Aug 21, 2021

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

Copy link
Member Author

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.

@kallewoof
Copy link
Member Author

@prayank23

Can we add this to getbalance?

Adding to getbalance sounds reasonable to me.

@jonatack

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

This requires the wallet so decoderawtransaction is out, I'm afraid. :)

@jonatack
Copy link
Contributor

(getbalances is the replacement for getbalance).

@kristapsk
Copy link
Contributor

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 Show resolved Hide resolved
src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
if (!wallet) return NullUniValue;
CWallet* const pwallet = wallet.get();

RPCTypeCheck(request.params, {UniValue::VSTR}, true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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?

@kallewoof
Copy link
Member Author

Thanks for all the feedback. I'm rewriting this as a separate pull request that adds the feature to getbalances. I'll potentially keep updating this PR as well, in case somebody prefers this variant.

@kallewoof
Copy link
Member Author

Opened alternative #22776 and updated this PR.

@kallewoof kallewoof changed the title wallet/rpc: add analyzerawtransaction RPC rpc/wallet: add analyzerawtransaction RPC Aug 23, 2021
@kallewoof kallewoof force-pushed the 202108-analyzerawtransaction branch 3 times, most recently from c42137c to f19676b Compare August 23, 2021 07:48
Copy link
Contributor

@kristapsk kristapsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK f19676b

@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 23, 2021

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #25621 (rpc/wallet: Add details and duplicate section for simulaterawtransaction by anibilthare)

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.

@kallewoof kallewoof force-pushed the 202108-analyzerawtransaction branch 2 times, most recently from 8dd19f4 to c3628da Compare August 24, 2021 11:39
Copy link
Contributor

@kristapsk kristapsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-ACK c3628da

@achow101
Copy link
Member

re-ACK bd52034

if isinstance(v, Decimal) or isinstance(vexp, Decimal):
v=Decimal(v)
vexp=Decimal(vexp)
vspan=Decimal(vspan)
Copy link
Member

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?

Copy link
Member Author

@kallewoof kallewoof Apr 26, 2022

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?

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request May 21, 2022
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request May 21, 2022
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
@kallewoof kallewoof force-pushed the 202108-analyzerawtransaction branch from bd52034 to b557f53 Compare July 15, 2022 07:23
@anibilthare
Copy link

Ack. Tested manually. Works fine.

I have also created a follow up PR which extends the functionality of this RPC.

@achow101
Copy link
Member

ACK b557f53

Comment on lines 657 to 662
// 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);
Copy link
Member

@furszy furszy Jul 26, 2022

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

Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member Author

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

Copy link
Contributor

@w0xlt w0xlt left a 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])
Copy link
Contributor

@w0xlt w0xlt Jul 27, 2022

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.

Copy link
Member Author

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.

src/wallet/rpc/wallet.cpp Outdated Show resolved Hide resolved
@kallewoof kallewoof force-pushed the 202108-analyzerawtransaction branch from b557f53 to 66cb907 Compare August 2, 2022 01:11
@kallewoof kallewoof force-pushed the 202108-analyzerawtransaction branch from 66cb907 to ced8a3e Compare August 2, 2022 01:11
@achow101
Copy link
Member

achow101 commented Aug 4, 2022

re-ACK ced8a3e

Copy link
Contributor

@jonatack jonatack left a 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.

// 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;
Copy link
Contributor

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

Suggested change
auto& outpoint = txin.prevout;
const auto& outpoint = txin.prevout;

// everything in these
// Also populate new_utxos in case these are spent in later transactions

const auto hash = mtx.GetHash();
Copy link
Contributor

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)

Suggested change
const auto hash = mtx.GetHash();
const uint256 hash{mtx.GetHash()};

or

Suggested change
const auto hash = mtx.GetHash();
const uint256& hash = mtx.GetHash();

Copy link
Member Author

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.

Copy link
Contributor

@jonatack jonatack Aug 5, 2022

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.

},
{"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')"},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ced8a3e

Suggested change
{"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)"},

{
std::shared_ptr<const CWallet> const wallet = GetWalletForJSONRPCRequest(request);
if (!wallet) return NullUniValue;
const CWallet* pwallet = wallet.get();
Copy link
Contributor

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)

Suggested change
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.
@kallewoof kallewoof force-pushed the 202108-analyzerawtransaction branch from ced8a3e to db10cf8 Compare August 5, 2022 00:48
@kallewoof
Copy link
Member Author

Applied suggestions by @jonatack. Thanks for the feedback!

@jonatack
Copy link
Contributor

jonatack commented Aug 5, 2022

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)

@achow101
Copy link
Member

achow101 commented Aug 5, 2022

re-ACK db10cf8

@achow101 achow101 merged commit 35305c7 into bitcoin:master Aug 5, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 6, 2022
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
@kallewoof kallewoof deleted the 202108-analyzerawtransaction branch August 6, 2022 06:28
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",
Copy link
Member

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",
Copy link
Member

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",
Copy link
Member

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)) {
Copy link
Member

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"

Copy link
Member

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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is split?

Copy link
Member

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
Copy link
Member

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
Copy link
Member

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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be removed

@bitcoin bitcoin locked and limited conversation to collaborators Aug 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet