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

Fix V5 transaction txid (#392) #393

Merged
merged 1 commit into from
May 24, 2022
Merged

Conversation

LarryRuane
Copy link
Collaborator

Partially fixes #392
This is a shortcut fix. Instead of replicating the zip244 transaction
logic that's implemented in librustzcash:
zcash_primitives/src/transaction/components/orchard.rs

cheat by calling getblock with verbose level 1, which prints the txid
of each transaction. This currently requires calling getblock twice,
once for the raw hex (as we have always done), and again to get the
transaction IDs. An easy improvement would be to add the raw hex to
verbosity 1 result. (Or have a new verbosity that shows both.)

The unit tests don't pass with this change, but I wanted to make it
available for testing as soon as possible, we can fix the tests later.

@LarryRuane LarryRuane added the bug Something isn't working label May 23, 2022
@str4d
Copy link
Collaborator

str4d commented May 23, 2022

An easy improvement would be to add the raw hex to
verbosity 1 result.

We will not do this; it could greatly increase the size of verbosity = 1 RPC outputs, which could have negative performance effects on downstream users.

Copy link
Collaborator

@str4d str4d left a comment

Choose a reason for hiding this comment

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

Code changes LGTM, but I have not tested them.

common/common.go Outdated Show resolved Hide resolved
common/common.go Outdated Show resolved Hide resolved
common/common.go Outdated Show resolved Hide resolved
parser/transaction.go Show resolved Hide resolved
parser/transaction_test.go Show resolved Hide resolved
@LarryRuane
Copy link
Collaborator Author

An easy improvement would be to add the raw hex to
verbosity 1 result.

We will not do this; it could greatly increase the size of verbosity = 1 RPC outputs, which could have negative performance effects on downstream users.

Verbosity should be a bitfield.

@LarryRuane
Copy link
Collaborator Author

Force-pushed to address review comments, thanks str4d!

@str4d
Copy link
Collaborator

str4d commented May 24, 2022

Verbosity should be a bitfield.

Perhaps, but we'd need to be careful about how this interacts with upstream, since getblock will be assumed by the wider community to behave like Bitcoin Core does.

This is a shortcut fix. Instead of replicating the zip244 transaction
logic that's implemented in librustzcash:
zcash_primitives/src/transaction/components/orchard.rs

cheat by calling getblock with verbose level 1, which prints the txid
of each transaction. This currently requires calling getblock twice,
once for the raw hex (as we have always done), and again to get the
transaction IDs. An easy improvement would be to add the raw hex to
verbosity 1 result. (Or have a new verbosity that shows both.)
@LarryRuane
Copy link
Collaborator Author

One more force-push to fix a comment as suggested by @str4d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement ZIP 244 txid logic for v5 transactions
2 participants