Add regtest support to `Parameter` for Mobile SDKs #1054

pacu · 2023-11-30T00:00:34Z

codecov · 2023-11-30T00:23:07Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (efc0d7d) 66.17% compared to head (09bbc79) 66.24%.
Report is 2 commits behind head on main.

Files	Patch %	Lines
zcash_primitives/src/local_consensus.rs	92.30%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1054      +/-   ##
==========================================
+ Coverage   66.17%   66.24%   +0.07%     
==========================================
  Files         114      115       +1     
  Lines       11063    11089      +26     
==========================================
+ Hits         7321     7346      +25     
- Misses       3742     3743       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

str4d · 2023-12-04T15:09:06Z

zcash_primitives/src/consensus.rs

+    RegtestNetwork {
+        overwinter: Option<BlockHeight>,
+        sapling: Option<BlockHeight>,
+        blossom: Option<BlockHeight>,
+        heartwood: Option<BlockHeight>,
+        canopy: Option<BlockHeight>,
+        nu5: Option<BlockHeight>,
+    },


Instead of a transparent enum variant (which is harder to make changes to later, as its internals are in the public API), move this to a pub struct RegtestNetwork {..}. Then the enum variant can be Regtest(RegtestNetwork).

I'm also not convinced that we should add a Regtest variant to the main Network enum. zcash_primitives::consensus::Network is a consensus network type, and only the main and test networks are consensus networks. Regtest mode is solely for local testing purposes, and in particular depends on the full node; zcashd supports its definition of regtest, while zebrad has no support for any kind of regtest mode (though I recall hearing vague whispers of some kind of local test network support that might be coming soon - cc @teor2345 et al).

I think regtest support (or more generally, user-configurable test networks) should be provided with a separate type that downstreams can opt into, similar to what we do in zcashd, and that lives somewhere other than zcash_primitives::consensus (maybe zcash_primitives::testing):

pub enum ZcashdNetwork { Consensus(consensus::Network), RegTest(RegTestNetwork), }

We could even retire the "regtest" name if we want to instead define this as a fully locally-defined test network capability (where zcashd's regtest mode configures only a subset of the available options):

pub enum FullNodeParameters { Consensus(consensus::Network), Local(LocalNetwork), }

I think we're working on basic support for other networks, but I'm not sure when that will be scheduled or complete. We're focused on the shielded scanner right now.

@arya2 might also have something to add here.

Thanks @str4d for your review. I agree with this quote below.

I think regtest support (or more generally, user-configurable test networks) should be provided with a separate type that downstreams can opt into, similar to what we do in zcashd, and that lives somewhere other than zcash_primitives::consensus (maybe zcash_primitives::testing)

The only thing I would differ is that we need to be able to use regtest from clients to have the integration tests like the advanced ReOrg tests. So we would need to access this RegtestNetwork types from the FFI.

I chose consensus::Network mostly for two reasons:

other crates were referring to it

The FFI uses that

And no others, so I'm definitely ok with refactoring this so that it's more convenient for today's reality.

The FFI uses it

To clarify, the FFI itself uses numeric constants for the kinds of networks it knows about, and then converts those into a consensus::Network. So the way for the iOS SDK to opt in to testing with a configurable local network is to change that helper function to use the new type that supports local testing. That change makes sense because the iOS SDK won't be able to do local testing without a bunch of other changes on its Swift side.

other crates were referring to it

If we change consensus::Network then some other crates using it may be forced to do that same bunch of work (depending on how they use the enum) that they don't want to do (perhaps because they are only concerned with the consensus networks).

I like the Consensus and LocalNetwork approach I'll give it a go and we can take it from there.

@str4d I think the changes I pushed might be heading into the right direction

zcash_primitives/src/lib.rs

nuttycom · 2024-01-05T16:58:29Z

@pacu sorry that we didn't get this in earlier; it's now conflicted, so can you please rebase and I'll re-review?

pacu · 2024-01-05T17:24:57Z

@pacu sorry that we didn't get this in earlier; it's now conflicted, so can you please rebase and I'll re-review?

No worries. I'll rebase it! Thanks

nuttycom

Sorry I missed this before, but the additions need rustdoc & CHANGELOG entries, and the feature flag needs to be added to Cargo.toml

zcash_primitives/src/local_consensus.rs

zcash_primitives/src/lib.rs

zcash_primitives/src/consensus.rs

zcash_primitives/CHANGELOG.md

zcash_primitives/src/consensus.rs

zcash_primitives/src/local_consensus.rs

nuttycom

utACK modulo a few additional documentation suggestions.

nuttycom · 2024-01-16T19:55:36Z

@pacu builds are currently failing?

pacu · 2024-01-17T19:58:48Z

@pacu builds are currently failing?

They don't fail for me locally but I'm not running an ubuntu target. It seemed intermittent to me.

do we have some script to build all the different targets or is that something you folks delegate to CI?

closes zcash#1051 Also: Adds encoding tests + cargo fmt Adds tests on ZIP-321 Adds coverage on new code Supports nu6 activation `local-consensus` feature is disabled by default

nuttycom

utACK 09bbc79

daira · 2024-01-18T23:45:52Z

zcash_primitives/Cargo.toml

@@ -132,6 +132,8 @@ unstable-nu6 = []
 ## Exposes early in-development features that are not yet planned for any network upgrade.
 zfuture = []

+## Exposes support for working with a local consensus (e.g. regtest


Suggested change

## Exposes support for working with a local consensus (e.g. regtest

## Exposes support for working with a local consensus (e.g. regtest).

Non-blocking.

daira · 2024-01-18T23:48:07Z

zcash_primitives/src/local_consensus.rs

+/// a `LocalNetwork` setup should define the activation heights
+/// of network upgrades. `None` is considered as "not activated"
+/// These heights are not validated. Callers shall initialized


Suggested change

/// a `LocalNetwork` setup should define the activation heights

/// of network upgrades. `None` is considered as "not activated"

/// These heights are not validated. Callers shall initialized

/// A `LocalNetwork` setup should define the activation heights

/// of network upgrades. `None` is considered as "not activated"

/// These heights are not validated. Callers should initialize

Non-blocking.

daira · 2024-01-18T23:49:15Z

zcash_primitives/src/local_consensus.rs

+///         nu6: Some(BlockHeight::from_u32(1)),
+///         #[cfg(feature = "zfuture")]
+///         z_future: Some(BlockHeight::from_u32(1)),
+///         };


Suggested change

/// };

/// };

Non-blocking.

daira

utACK, non-blocking doc-only suggestions.

pacu changed the title ~~Add regtest support to Parameter~~ Add regtest support to Parameter to support Mobile SDKs Nov 30, 2023

pacu changed the title ~~Add regtest support to Parameter to support Mobile SDKs~~ Add regtest support to Parameter for Mobile SDKs Nov 30, 2023

pacu mentioned this pull request Nov 30, 2023

REGTEST support for librustzcash pacu/zwcd#72

Closed

pacu force-pushed the regtest-support branch 4 times, most recently from 3b0af31 to e497fb6 Compare November 30, 2023 16:23

str4d reviewed Dec 4, 2023

View reviewed changes

pacu requested review from str4d and teor2345 December 6, 2023 22:27

pacu force-pushed the regtest-support branch from b35420d to 03c4590 Compare December 8, 2023 12:59

nuttycom reviewed Dec 11, 2023

View reviewed changes

zcash_primitives/src/lib.rs Outdated Show resolved Hide resolved

pacu requested a review from nuttycom December 12, 2023 18:26

pacu force-pushed the regtest-support branch 2 times, most recently from 5561dd0 to 39e1123 Compare January 5, 2024 21:36

nuttycom requested changes Jan 8, 2024

View reviewed changes

zcash_primitives/src/local_consensus.rs Outdated Show resolved Hide resolved

zcash_primitives/src/local_consensus.rs Show resolved Hide resolved

zcash_primitives/src/lib.rs Outdated Show resolved Hide resolved

zcash_primitives/src/consensus.rs Outdated Show resolved Hide resolved

pacu force-pushed the regtest-support branch from ed86bef to 83b985d Compare January 9, 2024 22:10

pacu requested a review from nuttycom January 9, 2024 22:11

nuttycom reviewed Jan 9, 2024

View reviewed changes

zcash_primitives/CHANGELOG.md Outdated Show resolved Hide resolved

nuttycom reviewed Jan 9, 2024

View reviewed changes

zcash_primitives/src/consensus.rs Show resolved Hide resolved

nuttycom reviewed Jan 9, 2024

View reviewed changes

zcash_primitives/src/local_consensus.rs Outdated Show resolved Hide resolved

nuttycom reviewed Jan 9, 2024

View reviewed changes

zcash_primitives/src/local_consensus.rs Show resolved Hide resolved

nuttycom previously approved these changes Jan 9, 2024

View reviewed changes

pacu dismissed nuttycom’s stale review via f71a625 January 11, 2024 15:34

pacu force-pushed the regtest-support branch from f71a625 to 5c28a57 Compare January 11, 2024 15:38

pacu requested a review from nuttycom January 11, 2024 15:39

Add Regtest variant of Network and Parameter

529fd3d

closes zcash#1051 Also: Adds encoding tests + cargo fmt Adds tests on ZIP-321 Adds coverage on new code Supports nu6 activation `local-consensus` feature is disabled by default

pacu force-pushed the regtest-support branch from 5c28a57 to 529fd3d Compare January 17, 2024 21:21

pacu and others added 2 commits January 17, 2024 18:55

PR Suggestion by @nuttycom: remove the FullNodeParameters enum

17f074d

Fix rustfmt

09bbc79

nuttycom approved these changes Jan 18, 2024

View reviewed changes

daira reviewed Jan 18, 2024

View reviewed changes

daira approved these changes Jan 19, 2024

View reviewed changes

nuttycom merged commit 28e36dc into zcash:main Jan 19, 2024
25 of 26 checks passed

pacu mentioned this pull request Jan 24, 2024

Wrap Regtest support on librustzcash pacu/zwcd#85

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add regtest support to `Parameter` for Mobile SDKs #1054

Add regtest support to `Parameter` for Mobile SDKs #1054

pacu commented Nov 30, 2023

codecov bot commented Nov 30, 2023 •

edited

str4d Dec 4, 2023 •

edited

teor2345 Dec 4, 2023

pacu Dec 4, 2023

str4d Dec 5, 2023

pacu Dec 6, 2023

pacu Dec 6, 2023

nuttycom commented Jan 5, 2024

pacu commented Jan 5, 2024

nuttycom left a comment

nuttycom left a comment

nuttycom commented Jan 16, 2024

pacu commented Jan 17, 2024

nuttycom left a comment •

edited

daira Jan 18, 2024 •

edited

daira Jan 18, 2024 •

edited

daira Jan 18, 2024

daira left a comment

	## Exposes support for working with a local consensus (e.g. regtest
	## Exposes support for working with a local consensus (e.g. regtest).

Add regtest support to Parameter for Mobile SDKs #1054

Add regtest support to Parameter for Mobile SDKs #1054

Conversation

pacu commented Nov 30, 2023

codecov bot commented Nov 30, 2023 • edited

Codecov Report

str4d Dec 4, 2023 • edited

Choose a reason for hiding this comment

teor2345 Dec 4, 2023

Choose a reason for hiding this comment

pacu Dec 4, 2023

Choose a reason for hiding this comment

str4d Dec 5, 2023

Choose a reason for hiding this comment

pacu Dec 6, 2023

Choose a reason for hiding this comment

pacu Dec 6, 2023

Choose a reason for hiding this comment

nuttycom commented Jan 5, 2024

pacu commented Jan 5, 2024

nuttycom left a comment

Choose a reason for hiding this comment

nuttycom left a comment

Choose a reason for hiding this comment

nuttycom commented Jan 16, 2024

pacu commented Jan 17, 2024

nuttycom left a comment • edited

Choose a reason for hiding this comment

daira Jan 18, 2024 • edited

Choose a reason for hiding this comment

daira Jan 18, 2024 • edited

Choose a reason for hiding this comment

daira Jan 18, 2024

Choose a reason for hiding this comment

daira left a comment

Choose a reason for hiding this comment

Add regtest support to `Parameter` for Mobile SDKs #1054

Add regtest support to `Parameter` for Mobile SDKs #1054

codecov bot commented Nov 30, 2023 •

edited

str4d Dec 4, 2023 •

edited

nuttycom left a comment •

edited

daira Jan 18, 2024 •

edited

daira Jan 18, 2024 •

edited