Migrate all `zcash_client_sqlite` pool tests to `zcash_client_backend` and make them generic over backend implementation #1533

willemolding · 2024-09-10T21:38:37Z

Moves the tests across, modifying where required and makes some additional changes to the data_api trait methods to support the test use-cases

willemolding · 2024-09-11T18:28:48Z

zcash_client_backend/src/data_api/testing/pool.rs

+/// The number of ephemeral addresses that can be safely reserved without observing any
+/// of them to be mined. This is the same as the gap limit in Bitcoin.
+#[cfg(feature = "transparent-inputs")]
+pub(crate) const GAP_LIMIT: u32 = 20;


Does this belong elsewhere maybe?

Yes; it can't live under zcash_client_backend::data_api_testing as that's only accessible with the test-dependencies feature flag.

willemolding · 2024-09-11T18:29:52Z

zcash_client_backend/src/data_api/testing/pool.rs

+            );
+            // previously it was hard-coded to a SqlClientError which could check the index
+            // not sure how best to handle this in a generic way
+            // Err(SqliteClientError::ReachedGapLimit(acct, bad_index))


Any thoughts on how to handle checking for specific errors like this in the back-end agnostic case?

str4d · 2024-09-12T17:01:11Z

zcash_client_backend/src/data_api.rs

+
+    /// Return the notes with a given value
+    #[cfg(any(test, feature = "test-dependencies"))]
+    fn get_notes(


In #1539 I'm moving these to a dedicated WalletTest trait so we can freely expose whatever internals are needed for testing purposes. (I'll handle any conflicts.)

str4d

The individual commits are structured as if they should be preserved, but they are currently unreviewable. Please fix the commits in the PR to be reviewable (which may amount to squashing all the commits together and then optionally splitting the resulting commit into functional parts).

str4d · 2024-09-12T17:04:37Z

zcash_client_sqlite/src/wallet.rs

@@ -3194,6 +3194,7 @@ pub mod testing {

        let results = stmt
            .query_and_then::<TransactionSummary<AccountId>, SqliteClientError, _, _>([], |row| {
+                let raw_tx: Vec<u8> = row.get("raw")?;


This will break (either loudly, or silently with an empty Vec which is a bad failure mode), as the raw column is nullable.

Suggested change

let raw_tx: Vec<u8> = row.get("raw")?;

let raw_tx: Option<Vec<u8>> = row.get("raw")?;

And indeed this was reverted in a later commit as broken. I cannot tell from the current commits why this was initially considered necessary, and why it is no longer needed.

str4d · 2024-09-12T17:07:53Z

zcash_client_sqlite/src/testing/pool.rs

@@ -570,1247 +570,127 @@ pub(crate) fn change_note_spends_succeed<T: ShieldedPoolTester>() {
    )
 }

-pub(crate) fn external_address_change_spends_detected_in_restore_from_seed<


Please fix this third commit (ad30ad6):

Commit message is remove the raw field on get_tx_history as it broke tests but the raw field is not removed.

A bunch of test content is deleted, with no visible move.

In order to review this PR, I need the code moves to be in the same commit, so that git show COMMIT --color-moved=zebra --color-moved-ws=allow-indentation-change works. Ideally the commit is as much a pure move as possible, with a fixup commit afterwards, but if the fixups are trivial then doing both at once is okayish.

str4d · 2024-09-12T17:09:10Z

zcash_client_backend/src/data_api/testing.rs

@@ -100,7 +100,6 @@ pub struct TransactionSummary<AccountId> {
    memo_count: usize,
    expired_unmined: bool,
    is_shielding: bool,
-    raw: Vec<u8>,


The raw removal is in the fourth commit (bb4dd30) instead of the third, but the fix is probably to rewrite the fourth commit's message (and separately fixup or remove the third commit per my previous comment).

str4d · 2024-09-12T17:11:04Z

zcash_client_backend/src/data_api/testing/pool.rs

+    // Now scan the block in which we received the note that was spent.
+    st.scan_cached_blocks(received_height, 1);
+
+    // Account balance should be the same.


This commit (bb4dd30) appears to be uncommenting a large amount of test code? I cannot figure out whether any of the uncommented code is part of the fix issue querying notes that the commit message says is going on.

Given that there are thousands of lines changed as a result of doing so, please uncomment the code in one commit, and then in a second commit make changes to it.

str4d · 2024-09-12T17:13:52Z

zcash_client_backend/src/data_api/testing/pool.rs

@@ -238,3 +240,2302 @@ pub fn send_single_step_proposed_transfer<T: ShieldedPoolTester>(
        Ok(_)
    );
 }
+
+// #[cfg(feature = "transparent-inputs")]
+// pub(crate) fn send_multi_step_proposed_transfer<T: ShieldedPoolTester>() {


Aha. Is what you did copy across all of the code, and comment it out at the destination in the same commit? Please do not do that, as the current approach makes it impossible to review the move.

Instead, either copy all the code as-is in one move (creating a commit that fails to compile) and then in subsequent commits apply the fixes, or move individual test functions over in one commit at a time.

nuttycom

Flushing a couple of comments from the review I started before @str4d's; I will re-review once the commit history is cleaned up to make it easier to see the moves.

nuttycom · 2024-09-11T15:23:04Z

zcash_client_backend/src/data_api/testing.rs

+                .block_height(),
+            h
+        );
+        assert_eq!(self.get_spendable_balance(account_id, 1), value);


This assertion being embedded here is a bit surprising, because a caller could reasonably expect to be able to call this in locations other than just at the start of a test.

nuttycom · 2024-09-11T15:25:40Z

zcash_client_backend/src/data_api/testing/pool.rs

@@ -238,3 +240,2302 @@ pub fn send_single_step_proposed_transfer<T: ShieldedPoolTester>(
        Ok(_)
    );
 }
+


As I'm reviewing this commit-by-commit, I'm surprised to see this copied in commented out here. I presume that it will be uncommented in a later commit, but it would b easier to evaluate the diff if the move were done in a single commit.

nuttycom · 2024-09-11T15:30:59Z

zcash_client_backend/src/data_api/testing/pool.rs

+        OvkPolicy::Sender,
+        &frobbed_proposal,
+    );
+    // TODO: Figure out how we want to handle different kinds of errors


This makes it clear that some of our error infrastructure needs to be refactored such that the persistence-specific errors are separate from the domain errors; right now SqliteClientError mixes the two. This is a somewhat invasive change though. It might make sense to just assert that there is an error for now, with the goal of later being able to uncomment this code once that refactoring has been performed; these TODOs can serve as motivation to get that done - maybe upgrade them to FIXMEs.

willemolding added 9 commits September 10, 2024 11:10

migrate 3 more tests across

0daca1f

adds new trait method to implement change_note_spends_succeed

526be27

remove the raw field on get_tx_history as it broke tests

ad30ad6

fix issue querying notes

bb4dd30

add error handling

6297428

migrate send_multi_step_proposed_transfer

3689aec

add data_api methods to allow fetching historical checkpoints

9d5e305

global fmt

bf29e8b

fix clippy lints

a648a52

willemolding marked this pull request as draft September 10, 2024 21:38

nuttycom requested review from nuttycom and str4d September 11, 2024 15:20

willemolding commented Sep 11, 2024

View reviewed changes

willemolding marked this pull request as ready for review September 11, 2024 18:31

feature gate orchard import

7f96b13

nuttycom mentioned this pull request Sep 12, 2024

zcash_client_backend: Post-merge cleanups to test framework extraction #1539

Open

str4d reviewed Sep 12, 2024

View reviewed changes

str4d requested changes Sep 12, 2024

View reviewed changes

nuttycom reviewed Sep 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate all `zcash_client_sqlite` pool tests to `zcash_client_backend` and make them generic over backend implementation #1533

Migrate all `zcash_client_sqlite` pool tests to `zcash_client_backend` and make them generic over backend implementation #1533

willemolding commented Sep 10, 2024

willemolding Sep 11, 2024

str4d Sep 13, 2024

willemolding Sep 11, 2024

str4d Sep 12, 2024

str4d left a comment

str4d Sep 12, 2024

str4d Sep 12, 2024

str4d Sep 12, 2024 •

edited

Loading

str4d Sep 12, 2024

str4d Sep 12, 2024

str4d Sep 12, 2024

nuttycom left a comment

nuttycom Sep 11, 2024

nuttycom Sep 11, 2024

nuttycom Sep 11, 2024

	let raw_tx: Vec<u8> = row.get("raw")?;
	let raw_tx: Option<Vec<u8>> = row.get("raw")?;

Migrate all zcash_client_sqlite pool tests to zcash_client_backend and make them generic over backend implementation #1533

Are you sure you want to change the base?

Migrate all zcash_client_sqlite pool tests to zcash_client_backend and make them generic over backend implementation #1533

Conversation

willemolding commented Sep 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

str4d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

str4d Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nuttycom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Migrate all `zcash_client_sqlite` pool tests to `zcash_client_backend` and make them generic over backend implementation #1533

Migrate all `zcash_client_sqlite` pool tests to `zcash_client_backend` and make them generic over backend implementation #1533

str4d Sep 12, 2024 •

edited

Loading