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

Implement a WtxId struct, and use it in Zebra's external network protocol #2618

Merged
merged 9 commits into from
Aug 16, 2021

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Aug 13, 2021

Motivation

In order to handle wide transaction IDs from the network, Zebra needs to:

  • split them into id and auth_data fields
  • use them when serializing and deserializing inventory hashes (in inv, getdata, and notfound messages)

Specifications

https://zips.z.cash/zip-0239#specification

Solution

Changes

  • Add a WtxId type for wide transaction IDs
  • Add conversions between transaction IDs and bytes
  • Use the WtxId type in external network protocol messages

Bug Fixes

  • Make the AuthDigest display order match transaction IDs

Tests

  • Move a transaction::Hash test to tests module
  • Add display order tests for transaction IDs and auth digests
  • Add round-trip byte and struct tests for transaction IDs and auth digests

This is part of ticket #2449, but it does not close that ticket.

Review

Anyone can review this PR.
It's blocking all of the mempool work that needs wide transaction IDs.

It's based on PR #2547, so it might need rebasing after that PR merges to main.

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

Follow Up Work

Create an UnminedTxId enum, to abstract over unmined v4 and v5 transactions in Zebra's internal network protocol

  • UnminedTxId is also blocking all of the mempool work that needs wide transaction IDs

This change is Reviewable

@teor2345 teor2345 added A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement NU-5 Network Upgrade: NU5 specific tasks P-Medium A-network Area: Network protocol updates or fixes labels Aug 13, 2021
@teor2345 teor2345 requested a review from a team August 13, 2021 07:16
@teor2345 teor2345 self-assigned this Aug 13, 2021
@teor2345 teor2345 changed the title Implement a WtxId struct, and use it in the network protocol Implement a WtxId struct, and use it in Zebra's external network protocol Aug 13, 2021
Base automatically changed from auth-digest to main August 13, 2021 16:58
@teor2345 teor2345 force-pushed the wtxid-struct-network branch from 4d08bec to fd803c2 Compare August 16, 2021 03:04
Copy link
Collaborator

@conradoplg conradoplg left a comment

Choose a reason for hiding this comment

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

Looks good, I liked the new code()

@teor2345 teor2345 enabled auto-merge (squash) August 16, 2021 20:56
@teor2345 teor2345 merged commit 6c86c8d into main Aug 16, 2021
@teor2345 teor2345 deleted the wtxid-struct-network branch August 16, 2021 21:26
Copy link
Contributor

@dconnolly dconnolly left a comment

Choose a reason for hiding this comment

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

👍

/// [ZIP-244]: https://zips.z.cash/zip-0244..
#[derive(Copy, Clone, Eq, PartialEq, Debug)]
/// Note: Zebra displays transaction and block hashes in big-endian byte-order,
/// following the u256 convention set by Bitcoin and zcashd.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
let mut reversed_bytes = self.0;
reversed_bytes.reverse();
f.write_str(&hex::encode(&reversed_bytes))
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

// TODO: Actually handle this variant once the mempool is implemented
Wtx([u8; 64]),
// TODO: Actually handle this variant once the mempool is implemented (#2449)
Wtx(transaction::WtxId),
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-network Area: Network protocol updates or fixes A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement NU-5 Network Upgrade: NU5 specific tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants