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

Add remaining value balance check #2565

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented Aug 4, 2021

Motivation

After #2561 we are in position to readd the remaining value balance consensus rule.

Designs

https://zebra.zfnd.org/dev/rfcs/0012-value-pools.html#definitions

Solution

Readd the check.

Review

I will like @teor2345 can take a look, we have some tests failing.


This change is Reviewable

@oxarbitrage
Copy link
Contributor Author

We made progress with the UTXOs here but still have 4 tests failing here:

failures:

---- service::check::tests::utxo::accept_arbitrary_transparent_spend_from_previous_block stdout ----
proptest: Aborting shrinking after the PROPTEST_MAX_SHRINK_ITERS environment variable or ProptestConfig.max_shrink_iters iterations (set 64 to a large(r) value to shrink more; current configuration: 64 iterations)

The application panicked (crashed).
Message:  Test failed: assertion failed: `(left == right)` 
  left: `Err(CommitBlockError(InvalidRemainingTransparentValue { transaction_hash: transaction::Hash("edf3f453fcd34b6522d2dad4c03ebb334de46c4d0cc9d6cd04d2f8d8b4d3fbf1"), in_finalized_state: false }))`,
 right: `Ok(())` at zebra-state/src/service/check/tests/utxo.rs:284; minimal failing input: output = zebra_chain::transparent::Output, mut prevout_input = zebra_chain::transparent::Input, use_finalized_state_output = false, mut use_finalized_state_spend = true
	successes: 0
	local rejects: 0
	global rejects: 0

Location: zebra-state/src/service/check/tests/utxo.rs:134

Backtrace omitted.
Run with RUST_BACKTRACE=1 environment variable to display it.
Run with RUST_BACKTRACE=full to include source snippets.

---- service::check::tests::utxo::accept_later_transparent_spend_from_this_block stdout ----
proptest: Aborting shrinking after the PROPTEST_MAX_SHRINK_ITERS environment variable or ProptestConfig.max_shrink_iters iterations (set 64 to a large(r) value to shrink more; current configuration: 64 iterations)

The application panicked (crashed).
Message:  Test failed: assertion failed: `(left == right)` 
  left: `Err(CommitBlockError(InvalidRemainingTransparentValue { transaction_hash: transaction::Hash("edf3f453fcd34b6522d2dad4c03ebb334de46c4d0cc9d6cd04d2f8d8b4d3fbf1"), in_finalized_state: false }))`,
 right: `Ok(())` at zebra-state/src/service/check/tests/utxo.rs:204; minimal failing input: output = zebra_chain::transparent::Output, mut prevout_input = zebra_chain::transparent::Input, use_finalized_state = false
	successes: 0
	local rejects: 0
	global rejects: 0

Location: zebra-state/src/service/check/tests/utxo.rs:134

Backtrace omitted.
Run with RUST_BACKTRACE=1 environment variable to display it.
Run with RUST_BACKTRACE=full to include source snippets.

---- service::check::tests::utxo::reject_duplicate_transparent_spend_in_same_chain_from_previous_block stdout ----
proptest: Aborting shrinking after the PROPTEST_MAX_SHRINK_ITERS environment variable or ProptestConfig.max_shrink_iters iterations (set 64 to a large(r) value to shrink more; current configuration: 64 iterations)

The application panicked (crashed).
Message:  Test failed: assertion failed: `(left == right)` 
  left: `Err(CommitBlockError(InvalidRemainingTransparentValue { transaction_hash: transaction::Hash("0ded6c7f7b30a09acf62f1beabafba4ca3aec6580a0c762a463c8ff577b041ff"), in_finalized_state: false }))`,
 right: `Ok(())` at zebra-state/src/service/check/tests/utxo.rs:593; minimal failing input: output = zebra_chain::transparent::Output, mut prevout_input1 = zebra_chain::transparent::Input, mut prevout_input2 = zebra_chain::transparent::Input, use_finalized_state_output = true, mut use_finalized_state_spend = false
	successes: 1
	local rejects: 0
	global rejects: 0

Location: zebra-state/src/service/check/tests/utxo.rs:134

Backtrace omitted.
Run with RUST_BACKTRACE=1 environment variable to display it.
Run with RUST_BACKTRACE=full to include source snippets.

---- service::non_finalized_state::tests::prop::rejection_restores_internal_state stdout ----
proptest: Aborting shrinking after the PROPTEST_MAX_SHRINK_ITERS environment variable or ProptestConfig.max_shrink_iters iterations (set 4 to a large(r) value to shrink more; current configuration: 4 iterations)

The application panicked (crashed).
Message:  Test failed: remaining value in the transparent transaction value pool MUST be nonnegative: transaction::Hash("45d034340e80d3089f5e70fb3048dc8be74a3f999c31583bbdbfa4f5cba9ab4c"), in finalized state: false; minimal failing input: (chain, valid_count, network, mut bad_block) = (alloc::vec::Vec<zebra_state::request::PreparedBlock><zebra_state::request::PreparedBlock>, len=104, 47, Mainnet, Block { height: Height(46), hash: 7c00b0087224aae89c52a861db11be8c4f45ff24739170186b302511740e383c })
	successes: 0
	local rejects: 4
		1 times at arbitrary v4 transfers with no spends and no outputs
		3 times at arbitrary v5 transfers with no spends and no outputs
	global rejects: 0

Location: zebra-state/src/service/non_finalized_state/tests/prop.rs:131

Backtrace omitted.
Run with RUST_BACKTRACE=1 environment variable to display it.
Run with RUST_BACKTRACE=full to include source snippets.


failures:
    service::check::tests::utxo::accept_arbitrary_transparent_spend_from_previous_block
    service::check::tests::utxo::accept_later_transparent_spend_from_this_block
    service::check::tests::utxo::reject_duplicate_transparent_spend_in_same_chain_from_previous_block
    service::non_finalized_state::tests::prop::rejection_restores_internal_state

@teor2345
Copy link
Contributor

teor2345 commented Aug 4, 2021

We made progress with the UTXOs here but still have 4 tests failing here:

That's because the proptests generate random values, which don't obey the remaining transaction value rule.

We had this fix on our list of tasks:
#2486 (review)

But I just started it yesterday, because of the mempool planning work and some complicated bugs in designs and reviews.

I should be able to push a PR which sets all the generated amounts to zero today. But we can't merge this PR until we have valid non-zero amounts. That might take me until tomorrow.

@teor2345
Copy link
Contributor

teor2345 commented Aug 4, 2021

Are you going to fix the transparent value balance signs, or should I?

Here are the changes we need to make to code we've already merged:
#2555 (comment)
#2555 (comment)

This should fix the transparent sign bug you're seeing in your draft code.

@teor2345 teor2345 changed the title Readd remaining value balance check Add remaining value balance check Aug 4, 2021
@teor2345
Copy link
Contributor

teor2345 commented Aug 4, 2021

I should be able to push a PR which sets all the generated amounts to zero today. But we can't merge this PR until we have valid non-zero amounts. That might take me until tomorrow.

If you rebase this PR on PR #2566, the tests should pass.

But I'll need more time to complete that PR - we need to test with valid non-zero amounts.

@teor2345
Copy link
Contributor

teor2345 commented Aug 4, 2021

Are you going to fix the transparent value balance signs, or should I?

Here are the changes we need to make to code we've already merged:
#2555 (comment)
#2555 (comment)

I update the docs here:
https://github.com/ZcashFoundation/zebra/pull/2566/files#diff-37e55893bd83d3e54e00fef8ec73dd922533d3d098952bc2747b8daad5bd03c1R679

We still need to flip the transparent sign in Transaction::transparent_value_balance and ValueBalance::remaining_transaction_value.

If you do get to it today, I can do it, because I need it to complete PR #2566. (Otherwise I'll have to swap signs in that as well.)

@oxarbitrage
Copy link
Contributor Author

I think we can close this one as it is replaced by #2566 where the check is re added and #2569 where the signs are changed.

@oxarbitrage oxarbitrage closed this Aug 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants