zebra icon indicating copy to clipboard operation
zebra copied to clipboard

bug: `getrawtransaction` output conflicts with `zcashd` output

Open str4d opened this issue 1 year ago • 5 comments

What happened?

zcashd's getrawtransaction API is inherited from Bitcoin Core, and thus its behaviour around general transaction properties (as opposed to Zcash-specific properties) is likely ossified within the wider ecosystem (in particular for exchanges). In particular, the way it sets the confirmations field has been that way for many years.

In https://github.com/zcash/zcash/commit/f381d4e0856b5f6ebba429119f3fb97f83983c51 zcashd brought in the Insight Block Explorer patches, which added a height field alongside the confirmations field. Thus everything I say below about how height is set applies equivalently to confirmations (just with different values as appropriate).

zcashd's getrawtransaction sets height in one of three ways:

  • If the transaction is in the main chain, height is set to the height of the block it was mined in. https://github.com/zcash/zcash/blob/0bcddb896c51f58771a20966a0b1f5e5110a7e52/src/main.cpp#L2085-L2088 https://github.com/zcash/zcash/blob/0bcddb896c51f58771a20966a0b1f5e5110a7e52/src/rpc/rawtransaction.cpp#L322-L329

  • If the transaction is in the mempool, height is omitted. https://github.com/zcash/zcash/blob/0bcddb896c51f58771a20966a0b1f5e5110a7e52/src/main.cpp#L2064-L2069 https://github.com/zcash/zcash/blob/0bcddb896c51f58771a20966a0b1f5e5110a7e52/src/rpc/rawtransaction.cpp#L322

  • If the transaction is not in the mempool and not in the main chain, but is still recognized due to being mined in a block that is currently orphaned (in a non-main chain), height is set to -1. https://github.com/zcash/zcash/blob/0bcddb896c51f58771a20966a0b1f5e5110a7e52/src/rpc/rawtransaction.cpp#L332-L335

zebrad does not implement this API, and instead sets height as follows:

  • If the transaction is in the main chain, height is set to the height of the block it was mined in. https://github.com/ZcashFoundation/zebra/blob/16168d762313a9d4c2af4c30cfc3e172c3291145/zebra-rpc/src/methods.rs#L1019-L1028 https://github.com/ZcashFoundation/zebra/blob/16168d762313a9d4c2af4c30cfc3e172c3291145/zebra-rpc/src/methods.rs#L1692-L1698

  • If the transaction is in the mempool, height is set to -1. https://github.com/ZcashFoundation/zebra/blob/16168d762313a9d4c2af4c30cfc3e172c3291145/zebra-rpc/src/methods.rs#L1004

  • If the transaction is not in the mempool and not in the main chain, but is still recognized due to being mined in a block that is currently orphaned (in a non-main chain), zebrad reports Err("transaction not found") because non-main chain blocks are not included in zebra_state::ReadResponse::Transaction. https://github.com/ZcashFoundation/zebra/blob/16168d762313a9d4c2af4c30cfc3e172c3291145/zebra-rpc/src/methods.rs#L1029-L1031

What were you doing when the issue happened?

Debugging lightwalletd, which itself is misusing the output of getrawtransaction from zcashd: https://github.com/zcash/librustzcash/pull/1473#discussion_r1701063435

Zebra logs

No response

Zebra Version

No response

Which operating systems does the issue happen on?

  • [ ] Linux
  • [ ] macOS
  • [ ] Windows
  • [ ] Other OS

OS details

No response

Additional information

No response

str4d avatar Aug 05 '24 17:08 str4d

This is also effectively a subset of #8646.

str4d avatar Aug 05 '24 17:08 str4d

The fact that zebrad behaves differently than zcashd, but presumably lightwalletd clients do not adapt their behaviour to the backing node type, is further evidence to me that we cannot fix this in a backward-compatible way and shouldn't worry about doing so when changing lightwalletd.

daira avatar Aug 05 '24 22:08 daira

We're going to wait until we're ready for NU6 testnet activation before prioritising this one

mpguerra avatar Aug 06 '24 16:08 mpguerra

We also identified that the error codes returned from getrawtransaction in the case that the transaction is not found do not correspond to those from zcashd:

~/work/lightwalletd on ᚠfix/get_transaction_notfound [$] via 🐹 v1.22.2 on ☁️   [email protected](us-east1)
✗ grpcurl -d @ na-lax.zcashd.zec.rocks:443 cash.z.wallet.sdk.rpc.CompactTxStreamer/GetTransaction <<EOM
{"hash": "OVmYYbd17odbutVyi9YaHZqAbyBzixVG6q/wTdwgEFM="}
EOM
ERROR:
  Code: Unknown
  Message: -5: No such mempool or blockchain transaction. Use gettransaction for wallet transactions.

~/work/lightwalletd on ᚠfix/get_transaction_notfound [$] via 🐹 v1.22.2 on ☁️   [email protected](us-east1)
✗ grpcurl -d @ zec.rocks:443 cash.z.wallet.sdk.rpc.CompactTxStreamer/GetTransaction <<EOM
{"hash": "OVmYYbd17odbutVyi9YaHZqAbyBzixVG6q/wTdwgEFM="}
EOM
ERROR:
  Code: Unknown
  Message: 0: Transaction not found

Error codes should be drawn from https://github.com/zcash/zcash/blob/master/src/rpc/protocol.h#L32-L80

nuttycom avatar Aug 20 '24 19:08 nuttycom

  • If the transaction is not in the mempool and not in the main chain, but is still recognized due to being mined in a block that is currently orphaned (in a non-main chain), height is set to -1.

@str4d, since Zebra doesn't keep orphaned blocks below the rollback limit, do you think it'd be OK to return the error "No such mempool or blockchain transaction." in this case?

upbqdn avatar Oct 18 '24 12:10 upbqdn