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

Replace primitive_types with bigint #829

Closed
1 task
hdevalence opened this issue Aug 5, 2020 · 8 comments
Closed
1 task

Replace primitive_types with bigint #829

hdevalence opened this issue Aug 5, 2020 · 8 comments
Labels
A-dependencies Area: Dependency file updates A-rust Area: Updates to Rust code NU-5 Network Upgrade: NU5 specific tasks

Comments

@hdevalence
Copy link
Contributor

hdevalence commented Aug 5, 2020

Motivation

We want to use zcash_primitives for non-malleable transaction IDs in PR #2129. But our primitive_types dependency is incompatible with zcash_primitives.

Also, primitive_types has a fairly decent dependency footprint:

primitive-types v0.7.2
├── fixed-hash v0.6.1
│   ├── byteorder v1.3.4
│   ├── rand v0.7.3 (*)
│   ├── rustc-hex v2.1.0
│   └── static_assertions v1.1.0
├── impl-codec v0.4.2
│   └── parity-scale-codec v1.3.4
│       ├── arrayvec v0.5.1
│       ├── bitvec v0.17.4
│       │   ├── either v1.5.3
│       │   └── radium v0.3.0
│       ├── byte-slice-cast v0.3.5
│       └── serde v1.0.114 (*)
└── uint v0.8.4
    ├── byteorder v1.3.4
    ├── crunchy v0.2.2
    ├── rustc-hex v2.1.0
    └── static_assertions v1.1.0

This isn't a high priority compared to getting everything working, but it's one way we can prune the size of our dep tree.

Solution

We want to replace primitive_types with the bigint crate, because librustzcash also uses that dependency .

Required Code Changes

  • use bigint::U256 in zebra_chain::work

To calculate the block difficulty threshold from recent blocks, we need to perform the following operations on u256 values:

  • division
  • invert bits
    • this can be implemented as subtraction from the all-ones u256
  • addition
  • multiplication
  • comparison

Alternatives

Here are the alternative libraries I looked at:

@hdevalence hdevalence added A-dependencies Area: Dependency file updates E-med A-rust Area: Updates to Rust code C-cleanup Category: This is a cleanup labels Aug 5, 2020
@zfnd-bot zfnd-bot bot added this to To Do in 🦓 Aug 5, 2020
@teor2345
Copy link
Contributor

teor2345 commented Aug 5, 2020

To calculate the block difficulty threshold from recent blocks, we need to perform the following operations on u256 values:

  • division
  • invert bits
  • addition
  • multiplication
  • comparison

See #802 for details.

Here are the alternative libraries I looked at:

Let me know which one you'd prefer, or if you know of another library that does u256 division.

@teor2345
Copy link
Contributor

teor2345 commented Aug 6, 2020

See #838 of one example where we use division.

We also need to use division for calculating the average difficulty (median of recent blocks, scaled by the time range of those blocks).

@str4d
Copy link
Contributor

str4d commented Aug 6, 2020

We currently depend on bigint in zcash_history, so it would be nice to coalesce on a common U256 dependency that we can use there as well, so that it doesn't come into the zebra dependency tree when the FlyClient consensus rules are added.

@teor2345
Copy link
Contributor

teor2345 commented Aug 10, 2020

bigint has about half the dependencies of primitive-types, because it doesn't depend on impl-codec. (It also doesn't depend on fixed-hash, but those dependencies are duplicates anyway.)

So Zebra could switch to bigint if you want, or Zebra and zcashd could both switch to num-bigint.

@hdevalence
Copy link
Contributor Author

Changed milestone since I think that if we have some working code, fixing it is lower-priority than getting the rest of our functionality done.

@teor2345
Copy link
Contributor

I tried switching from primitive_types to bigint before starting some new difficulty work.

But bigint doesn't support u128 conversions (it has its own U128 type), which would have meant some bit twiddling.

That's not a huge amount of work, but it's out of scope right now, so I left it for later.

@daira
Copy link
Contributor

daira commented Oct 12, 2020

doesn't have invert bits

That can be implemented as subtraction from the all-ones u256.

@mpguerra mpguerra added this to No Estimate in Effort Affinity Grouping Jan 4, 2021
@mpguerra mpguerra removed this from the Wallet Support 💰 milestone Jan 5, 2021
@dconnolly dconnolly moved this from No Estimate to S - 3 in Effort Affinity Grouping Jan 21, 2021
@mpguerra mpguerra removed the E-med label Mar 23, 2021
@teor2345 teor2345 changed the title Remove dependency on primitive_types Replace primitive_types with bigint Jun 2, 2021
@teor2345 teor2345 moved this from S - 3 to XS - 2 in Effort Affinity Grouping Jun 11, 2021
@teor2345 teor2345 added this to the 2021 Sprint 12 milestone Jun 11, 2021
@teor2345 teor2345 added NU-5 Network Upgrade: NU5 specific tasks P-Medium and removed C-cleanup Category: This is a cleanup labels Jun 11, 2021
@mpguerra mpguerra removed this from the 2021 Sprint 12 milestone Jun 21, 2021
@teor2345
Copy link
Contributor

teor2345 commented Nov 4, 2021

Obsoleted by #2350

@teor2345 teor2345 closed this as completed Nov 4, 2021
🦓 automation moved this from To Do to Done Nov 4, 2021
Effort Affinity Grouping automation moved this from XS - 2 to Done Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dependencies Area: Dependency file updates A-rust Area: Updates to Rust code NU-5 Network Upgrade: NU5 specific tasks
Projects
No open projects
Development

No branches or pull requests

6 participants