bug: getrawtransaction output conflicts with zcashd output

[str4d]

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 zcash/zcash@f381d4e 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.
  

  zebra/zebra-rpc/src/methods.rs

  Lines 1019 to 1028 in 16168d7

  	zebra_state::ReadResponse::Transaction(Some(MinedTx {
  	tx,
  	height,
  	confirmations,
  	})) => Ok(GetRawTransaction::from_transaction(
  	tx,
  	Some(height),
  	confirmations,
  	verbose,
  	)),

  
  

  zebra/zebra-rpc/src/methods.rs

  Lines 1692 to 1698 in 16168d7

  	height: match height {
  	Some(height) => height
  	.0
  	.try_into()
  	.expect("valid block heights are limited to i32::MAX"),
  	None => -1,
  	},

• If the transaction is in the mempool, height is set to -1.
  

  zebra/zebra-rpc/src/methods.rs

  Line 1004 in 16168d7

  return Ok(GetRawTransaction::from_transaction(tx, None, 0, verbose));

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

  zebra/zebra-rpc/src/methods.rs

  Lines 1029 to 1031 in 16168d7

  	zebra_state::ReadResponse::Transaction(None) => {
  	Err("Transaction not found").map_server_error()
  	}

What were you doing when the issue happened?

Debugging lightwalletd, which itself is misusing the output of getrawtransaction from zcashd: zcash/librustzcash#1473 (comment)

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]

This is also effectively a subset of #8646.

[daira]

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.

[mpguerra]

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

[nuttycom]

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

[upbqdn]

• 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. Use ..." in this case?