Add `gettransaction` #36

str4d · 2025-03-15T11:44:56Z

Closes #38.

str4d · 2025-06-17T15:54:40Z

zallet/src/components/json_rpc/balance.rs

+            }
+        }
+
+        // TODO: Fix bug in `zcashd` where we forgot to add Orchard here.


Fixed in zcash/zcash#7035.

Removed TODO at ac1011a

Closes #38.

nuttycom

It seems to me that some of the balance computation performed here cannot be made reliable in zallet. I question whether it's even a good idea to implement this method.

nuttycom · 2025-06-20T19:56:25Z

zallet/src/components/json_rpc/balance.rs

+                    SELECT 1
+                    FROM addresses
+                    JOIN accounts ON account_id = accounts.id
+                    WHERE address = :address


blocking: This should be comparing to cached_transparent_receiver_address, not address.

nuttycom · 2025-06-20T19:56:55Z

zallet/src/components/json_rpc/balance.rs

+                    WHERE address = :address
+                    AND (
+                        :allow_either = 1
+                        OR has_spend_key = :has_spend_key


Suggested change

OR has_spend_key = :has_spend_key

OR accounts.has_spend_key = :has_spend_key

The has_spend_key boolean value isn't really trustworthy; after #152 is complete the determination about whether spending keys are available should be made by querying the spending key tables. Also, what are the semantics of IsMine for P2SH outputs where you hold one, but not all, of the keys?

Also, what are the semantics of IsMine for P2SH outputs where you hold one, but not all, of the keys?

false: https://github.com/zcash/zcash/blob/18238d90cd0b810f5b07d5aaa1338126aa128c06/src/script/ismine.cpp#L94

// Only consider transactions "mine" if we own ALL the // keys involved. Multi-signature transactions that are // partially owned (somebody else has a key that can spend // them) enable spend-out-from-under-you attacks, especially // in shared-wallet situations.

Please reproduce that comment as documentation (suggested above).

8aca24f and 790f4a2

@oxarbitrage just a note, as you're working on this at the moment: given the balance problems described in https://github.com/zcash/wallet/pull/36/files#diff-aa4c5eb784ec34fa821ac213851b030ecd0d609b8d5b9f94252db39e476b68f6R162-R164 we discussed yesterday that this RPC will not be part of Zallet alpha. We don't want to include buggy methods in Zallet, and so instead we will modify z_viewtransaction to satisfy all of the use cases of this method in a non-buggy fashion, and attempt to get users to migrate to that.

That's fine, the changes i made were trivial and in case we wanted to go forward with this. If we don't have it yet, can we open an issue to modify z_viewtransaction and close this PR ?

Yes; we won't close this PR but we'll probably move it back to draft and keep it in our pocket in case the push to get users to migrate to z_viewtransaction is unsuccessful. I need to re-review what is reported by gettransaction and make a concrete plan for incorporating that data (but in a way that doesn't reproduce zcashd's bugs) into z_viewtransaction.

nuttycom · 2025-06-20T20:05:49Z

zallet/src/components/json_rpc/balance.rs

+/// Equivalent to [`CTransaction::GetValueOut`] in `zcashd`.
+///
+/// [`CTransaction::GetValueOut`]: https://github.com/zcash/zcash/blob/2352fbc1ed650ac4369006bea11f7f20ee046b84/src/primitives/transaction.cpp#L214
+pub(super) fn wtx_get_value_out(tx: &Transaction) -> Option<Zatoshis> {


The semantics of this whole method are very weird; how would someone reasonably use this value?

Can you provide a suggestion on what to do here?

Notice the TODO at https://github.com/zcash/wallet/pull/36/files#diff-aa4c5eb784ec34fa821ac213851b030ecd0d609b8d5b9f94252db39e476b68f6R191-R193 , just before the only call to this method:

// TODO: Alter the semantics here to instead use the concrete fee (spends - outputs). // In particular, for v6 txs this should equal the fee field, and it wouldn't with zcashd semantics. // See also https://github.com/zcash/zcash/issues/6821

I think the best way to proceed is to remove wtx_get_value_out and fix that TODO in the way it suggests, and so avoid introducing an equivalent issue to zcash/zcash#6821 in Zallet's gettransaction (if we decide to implement it). Note that #6821 is unequivocally a bug, not just a quirk.

nuttycom · 2025-06-20T20:09:52Z

zallet/src/components/json_rpc/balance.rs

+                if let Some(txout) = wallet
+                    .get_transaction(*txin.prevout.txid())?
+                    .as_ref()
+                    .and_then(|prev_tx| prev_tx.transparent_bundle())
+                    .and_then(|bundle| bundle.vout.get(txin.prevout.n() as usize))


bug: This seems unusable in the context of zallet; in zcashd, this behavior was okay because you could be confident that you had access to the transaction inputs; here, this will just silently ignore any value for inputs for which you don't have the full transaction.

Can you provide a suggestion on what to do here?

I think we should just omit fields that we can't calculate from the RPC output. These internal methods, if they are retained, should do whatever is needed to implement that.

daira · 2025-07-02T14:14:44Z

zallet/src/components/json_rpc/balance.rs

+        // TODO: Use `zcash_script` to discover other ways the output might belong to
+        // the wallet (like `IsMine(CScript)` does in `zcashd`).
+        None => Ok(false),


This is going to cause anxiety when zallet reports a lower balance than zcashd did, because it isn't seeing some outputs as "mine".

Indeed, which is why the TODO is present (so that we can detect IsMine the same way as zcashd)

daira · 2025-07-20T08:55:10Z

zallet/src/components/json_rpc/balance.rs

+use crate::components::database::DbConnection;
+
+/// Coinbase transaction outputs can only be spent after this number of new blocks
+/// (network rule).


Suggested change

/// (network rule).

/// (consensus rule).

The term "network rule" is specific to Bitcoin, and means the same thing as "consensus rule" in Bitcoin (sources: "Consensus" on the Bitcoin Wiki, and BIP 30 which uses "network rule" to mean "consensus rule"). Zcash documentation has always avoided this synonym and only used "consensus rule".

zallet/src/components/json_rpc/balance.rs

nuttycom · 2025-08-20T14:36:24Z

zallet/src/components/json_rpc/methods/get_transaction.rs

+    // `gettransaction` was never altered to take wallet shielded notes into account.
+    // As such, its `amount` and `fee` fields are calculated as if the wallet only has
+    // transparent addresses.


blocking: This has been a potential source of loss of funds for years, and we should not reproduce these bugs. A naive user who has received both shielded funds and transparent funds in the transaction may observe incorrect values, and I think that we should consider this kind of footgun unacceptable.

Instead of modifying the semantics of this method, we should not include it in the Zallet alpha, and instead provide a replacement in z_viewtransaction that we can encourage users to migrate to instead.

oxarbitrage · 2025-08-20T15:10:52Z

I applied some of the suggestions that were pending. Happy to squash everything at the end.

90cb282
It seems NB is short for "nota bene", Latin for "note well". This was too obscure for me, i changed it but can go back or change to something else.

daira · 2025-08-31T21:20:31Z

zallet/src/components/json_rpc/methods/get_transaction.rs

+
+    walletconflicts: Vec<String>,
+
+    /// The transaction time in seconds since epoch (1 Jan 1970 GMT).


Suggested change

/// The transaction time in seconds since epoch (1 Jan 1970 GMT).

/// The transaction time as a Unix timestamp (UTC seconds since the 1 Jan 1970 UTC epoch).

UTC has been defined to maintain a whole-second offset from TAI since 1972. "GMT" is ambiguous between UTC and UT1; the latter does not maintain a whole-second offset from TAI and can be different from UTC by ±0.9s. Also, "UTC seconds" ignores leap seconds.

daira · 2025-08-31T21:21:16Z

zallet/src/components/json_rpc/methods/get_transaction.rs

+    /// The transaction time in seconds since epoch (1 Jan 1970 GMT).
+    time: u64,
+
+    /// The time received in seconds since epoch (1 Jan 1970 GMT).


Suggested change

/// The time received in seconds since epoch (1 Jan 1970 GMT).

/// The time received as a Unix timestamp (UTC seconds since the 1 Jan 1970 UTC epoch).

daira · 2025-08-31T22:37:50Z

zallet/src/components/json_rpc/balance.rs

+///
+/// - `None`      : not in blockchain, and not in memory pool (conflicted transaction)
+/// - `Some(0)`   : in memory pool, waiting to be included in a block (never returned if `as_of_height` is set)
+/// - `Some(1..)` : this many blocks deep in the main chain


Suggested change

/// - `Some(1..)` : this many blocks deep in the main chain

/// - `Some(1..)` : this many blocks deep in the main chain, where `Some(1)` represents the tip

daira

Reviewed with comments.

daira · 2025-08-31T22:43:03Z

It seems NB is short for "nota bene", Latin for "note well". This was too obscure for me, i changed it but can go back or change to something else.

NB used to be quite common at least in British English; I remember using it quite a bit but it has fallen out of favour. Google Ngrams confirms this impression.

str4d force-pushed the rpc-transaction branch from 80686b2 to d01c42f Compare March 15, 2025 11:55

str4d force-pushed the rpc-transaction branch from d01c42f to e968d90 Compare April 10, 2025 11:25

str4d force-pushed the rpc-transaction branch from e968d90 to ce59e5b Compare April 25, 2025 23:21

str4d force-pushed the rpc-transaction branch 3 times, most recently from 99f4a35 to a412e09 Compare May 12, 2025 20:47

str4d added the A-rpc-interface Area: RPC interface label May 14, 2025

str4d force-pushed the rpc-transaction branch from a412e09 to 2b94d4a Compare May 19, 2025 20:45

str4d changed the base branch from main to 38-z_viewtransaction May 19, 2025 20:45

str4d changed the title ~~Add gettransaction and z_viewtransaction~~ Add gettransaction May 19, 2025

str4d mentioned this pull request May 20, 2025

rpc: Implement getwalletinfo #55

Open

Base automatically changed from 38-z_viewtransaction to main May 23, 2025 23:32

str4d force-pushed the rpc-transaction branch from 2b94d4a to eb6e543 Compare June 11, 2025 22:08

str4d marked this pull request as ready for review June 11, 2025 22:09

str4d force-pushed the rpc-transaction branch 3 times, most recently from 9f8b66c to e3eb615 Compare June 12, 2025 14:56

str4d changed the base branch from main to rust-updates June 12, 2025 14:56

Base automatically changed from rust-updates to main June 12, 2025 17:30

str4d commented Jun 17, 2025

View reviewed changes

Add gettransaction

8e2554b

Closes #38.

nuttycom force-pushed the rpc-transaction branch from e3eb615 to 8e2554b Compare June 20, 2025 19:28

nuttycom requested changes Jun 20, 2025

View reviewed changes

daira reviewed Jul 2, 2025

View reviewed changes

daira reviewed Jul 20, 2025

View reviewed changes

zallet/src/components/json_rpc/balance.rs Show resolved Hide resolved

oxarbitrage added 4 commits August 20, 2025 10:48

fix query

8aca24f

remove TODO

ac1011a

fix docs

790f4a2

minor doc change

90cb282

nuttycom reviewed Aug 20, 2025

View reviewed changes

oxarbitrage marked this pull request as draft August 23, 2025 11:32

daira reviewed Aug 31, 2025

View reviewed changes

str4d mentioned this pull request Sep 3, 2025

rpc: Add transparent information to z_viewtransaction #216

Merged

pacu moved this to In progress in Zallet Contributor's Board Oct 30, 2025

pacu added this to Zallet Contributor's Board Oct 30, 2025

	OR has_spend_key = :has_spend_key
	OR accounts.has_spend_key = :has_spend_key


		walletconflicts: Vec<String>,

		/// The transaction time in seconds since epoch (1 Jan 1970 GMT).

	/// The transaction time in seconds since epoch (1 Jan 1970 GMT).
	/// The transaction time as a Unix timestamp (UTC seconds since the 1 Jan 1970 UTC epoch).

	/// The time received in seconds since epoch (1 Jan 1970 GMT).
	/// The time received as a Unix timestamp (UTC seconds since the 1 Jan 1970 UTC epoch).

	/// - `Some(1..)` : this many blocks deep in the main chain
	/// - `Some(1..)` : this many blocks deep in the main chain, where `Some(1)` represents the tip

Add gettransaction #36

Are you sure you want to change the base?

Add gettransaction #36

Uh oh!

Conversation

str4d commented Mar 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nuttycom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daira Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nuttycom Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daira Aug 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daira Jul 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nuttycom Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oxarbitrage commented Aug 20, 2025

Uh oh!

daira Aug 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daira Aug 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daira left a comment

Choose a reason for hiding this comment

Add `gettransaction` #36

Add `gettransaction` #36

str4d commented Mar 15, 2025 •

edited

Loading

daira Jul 2, 2025 •

edited

Loading

nuttycom Aug 20, 2025 •

edited

Loading

daira Aug 31, 2025 •

edited

Loading

daira Jul 20, 2025 •

edited

Loading

nuttycom Aug 20, 2025 •

edited

Loading

daira Aug 31, 2025 •

edited

Loading

daira Aug 31, 2025 •

edited

Loading