-
Notifications
You must be signed in to change notification settings - Fork 38
phase out pseudorandom test-case generators #543
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
phase out pseudorandom test-case generators #543
Conversation
aszepieniec
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call to pause and wait for feedback, because to be honest I do not like these changes.
Be careful to avoid changing tests that involve producing proofs. Because producing proofs is expensive and we would like to avoid doing it unless absolutely necessary. (This is why we derandomized so many of them.) Mind you, we will generate new proofs if there is a compelling reason to do so.
For test helper functions, replacing a seed or rng argument with a test runner does not constitute an improvement. Instead, the test helper function should be redesigned. And I'm not sure there is a single formula that applies to all. Perhaps it should return a BoxedStrategy instead whatever it returns now, and the derandomization should happen on the side of the caller? Come to think of it, it might be useful to have a helper function that takes a Strategy and returns a deterministic sample. In fact, given that prop_map and prop_compose exist, we probably should not be passing test runners as function arguments anywhere.
I understand the idea, but not sure I will apply it correctly on my own: could you add an example or two for the reference! |
I was considering to add a macros for this, but I felt that such a facility will hinder further switching to the actual |
For instance, in |
Macros are okay as long as they are well documented. (Sometimes it can be tricky to discern how they work.) |
|
@aszepieniec |
@aszepieniec |
|
@aszepieniec |
|
(Side note: not all your tags are coming through. Feel free to tag me again if some time passes without sign of life from me.)
Yes. Although if the object in question is not involved in any deterministic tests then it's preferable to use whatever shortcuts
If I understand correctly (please correct any misunderstandings):
I would say (1) is good in principle: it makes the test helper usable in both contexts. But is it actually used on both contexts (or is that the plan)? If the answer is yes then let's go ahead with this change and regenerate proofs as necessary. As for (2), the fact that it was not derandomizable suggests that it was not ever used in deterministic tests. So there is no need to worry about invalidating deterministic proofs because there are none. However, I might be missing something but I don't see how this change makes the function in question more friendly towards I think (3) suffers from largely the same problem. Replacing There is more to be said about (3), namely that I think this example (not your proposed changes but the thing being changed) is emblematic of a more profound deficiency, which I hope can be resolved (or at least identified) simultaneously with moving reliance more on |
aszepieniec
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fabulous work, well done! By and large, this is a big step in what is absolutely the right direction.
I am requesting a few changes, but obliging them should take a negligible effort compared to what you've done already. Some change requests live inline. What follows here is general stuff.
I question the wisdom of including proptest failing instances for tests that were dramatically refactored since encountering that failure. If it's a small change, then the failing instances can catch regressions (which is good!) but if it's a dramatic change then catching regressions is unlikely and I wonder whether it makes sense to a) pollute the git environment, and b) reduce the speed for running the test suite.
You marked with #proofs_presaved_compat lines that look weird until you realize you want to reuse deterministic proofs. Thanks for the consideration, but it looks like you've already invalidated the existing proofs with changes further upstream in test_shared/. Going forward I suggest to fix these lines in the most natural way and I will find a way to re-generate all necessary proofs.
Some proptests use the proptest! macro, which takes some deciphering. I wonder if they can all be re-written to use the #[proptest] macro instead? If yes, that's preferable because it's a) consistent with other tests that are already present, and b) (IMHO) easier to read. But if not, it's okay to leave as-is.
There are a few proptest::prop_compose! functions. Please find a consistent naming pattern.
You marked with #arbitraryHashSetIterator lines that are equivalent obtaining an arbitrary HashSet from the proptest/test_strategy framework. Besides one comment I left inline about derandomizing it, I would suggest to factor out a helper struct and implement an appropriate arbitrary strategy for it.
Tests which require no structure in their input and intake only crypto randomness have no chance to have an advantage switching to proptest as it kind of searching an input space which feels pointless in such a case but bring overhead. Boundary tests seems to be a better thing (like trying identity elements, order, prime, maximal).
Agreed.
| #[strategy([arb(); 32])] seed: [u8; 32], | ||
| ) { | ||
| let mut rng = StdRng::from_seed(seed); | ||
| // TODO is it ok to rely on the arbitrary nature of `.iter()` for `HashSet`? #arbitraryHashSetIterator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it ok to rely on the arbitrary nature of
.iter()forHashSet? #arbitraryHashSetIterator
That's a great question, and sadly I think the answer is "no".
A LLM tells me:
the randomness [for
HashSet] is not deterministic by default. Each time you run your program, the HashSet will use a new random seed. This means that the internal order of elements (as seen when iterating) and the hash values themselves will differ between runs, even if the same data is inserted in the same order.
which is great if we want to try a whole bunch of independently drawn samples from the same distribution. But that would undermine the other reason to use proptest, which is shrinking when inputs fail. To enable that, you need determinism.
You can use HashSet::with_hasher to derandomize the HashSet. However, I'm not sure how to use that in combination with .collect().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it was the line of reasoning I was following too, and counter-arguments were quite weak. Take a look at a solution I found. Feels succinct and elegant, I guess even that additional helper for this would be excessive.
That crossed my mind too, though I feel lack of knowledge on
I was moving to
Yeah it's good for a helper, but the hashtag marks not an equivalent (as that hits the local rejection limit -- see the notes in the first comment for the PR, I put there a draft for the merge commit message).
Will take another look, though it seems |
I think with is the natural preposition that comes with compose. However, the thing that follows is not the thing being composed. What is being composed is strategies, and that part is left out. I'm just thinking out loud here. This is how I might subvocalize things: Prop-compose (a map of strategies resulting in) Based on that I think with is the right choice. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aszepieniec
So this should become propcompose_transactionkernel_with_records_pa?
pub fn propcompose_transaction_kernel_with_nums_of_inputs_outputs_pa(
num_inputs: usize,
num_outputs: usize,
num_public_announcements: usize,
) (
inputs in collection::vec(crate::util_types::test_shared::mutator_set::propcompose_removal_record(), num_inputs),
outputs in collection::vec(arb::<AdditionRecord>(), num_outputs),
public_announcements in collection::vec(collection::vec(arb::<BFieldElement>(), 10..59), num_public_announcements).prop_map(
|vecvec| vecvec.into_iter().map(|message| PublicAnnouncement { message }).collect_vec()
),
fee in arb::<NativeCurrencyAmount>(),
coinbase in arb::<Option<NativeCurrencyAmount>>(),
timestamp in arb::<Timestamp>(),
mutator_set_hash in arb::<Digest>(),
merge_bit in any::<bool>(),
) -> TransactionKernelI mean I was moving to what I described to have something easy to handle (and thank you for helping in this!). And my thought was like: if we will need a same Strategy that also takes a fixed fee or timestamp than it's easy to append those to the name or reverse and name the fields which are fuzzyied by the Strategy. So one would get maximum info from the name of the function and a peek to the output value type.
That doesn't sit well either :/ Ideally there is a struct that holds inputs, outputs, and public announcements, and nothing else. But I think it's silly to introduce that struct in order to improve the names of test helper functions. Come to think of it, it's not even the inputs, outputs, and public announcements per se -- it's their numbers. So maybe something like |
|
@aszepieniec pub fn propcompose_of_mutator(
inputs: Vec<RemovalRecord>,
outputs: Vec<AdditionRecord>,
fee: NativeCurrencyAmount,
timestamp: Timestamp,
) (mutator_set_hash in arb::<Digest>()) -> TransactionKernel {
TransactionKernelProxy {
inputs: inputs.clone(),
outputs: outputs.clone(),
public_announcements: vec![],
fee,
timestamp,
coinbase: None,
mutator_set_hash,
merge_bit: false,
}
.into_kernel()
}I omitted the composition result in the name since it's in the vocally named module and |
|
Following the same pattern it would be |
|
@aszepieniec
Okay, will do this way. Though for me personally that is a confusing
approach: in the previous case _lengths_ were fixed via the args, and
here it's the thing which is being fuzzed. Also I remembered I was
trying to be inline with that *arbitrary_with* method where "with" is
again fixing the args from fuzzing. And I lack the feeling of the
language which you have since mine English is meh. X)
…On Fri, Apr 18 2025 at 09:10:19 AM -0700, aszepieniec ***@***.***> wrote:
Following the same pattern it would be
propcompose_transaction_kernel_with_mutator_set_hash, I think. That
sounds good to my ears, and is appropriately descriptive and
accurate. If this name is too long, we can use the abbreviation "ms"
for mutator set, which is used elsewhere too.
—
Reply to this email directly, view it on GitHub
<#543 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/APXLOT7U6DE2LMBNQIGBM2322EPWXAVCNFSM6AAAAAB2LEUFPSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQMJVG4ZDIMJYHE>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
*aszepieniec*
left a comment
(Neptune-Crypto/neptune-core#543)
<#543 (comment)>
Following the same pattern it would be
propcompose_transaction_kernel_with_mutator_set_hash, I think. That
sounds good to my ears, and is appropriately descriptive and
accurate. If this name is too long, we can use the abbreviation "ms"
for mutator set, which is used elsewhere too.
—
Reply to this email directly, view it on GitHub
<#543 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/APXLOT7U6DE2LMBNQIGBM2322EPWXAVCNFSM6AAAAAB2LEUFPSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQMJVG4ZDIMJYHE>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Right, that's a great point that I missed. The roles of the argument have switched. So what I proposed is inconsistent. So let's go back to the drawing board. The question revolves around the arguments/fields which are fields of I can't think of a good name for a distinguisher between these two categories. How about "basic kernel info"? I would find it a little confusing that we introduce that term for just this function. The feature that marks this collection of fields is that all transactions have them and for someone looking at the transaction graph these fields are the interesting ones. (Coinbase is also interesting but a) it has no origin and b) transactions with set coinbase fields are rare.) So in light of that, maybe something along the lines of "standard transaction data". |
b362e83 to
c4b3357
Compare
aszepieniec
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left two minor comments inline.
There are still some pseudorandom_* methods left. Are you planning to phase those out in a future PR, or is there a reason why they cannot be made into strategies?
Tomorrow I will have access to a big machine which will be able to run the test suite and generate proofs. Assuming everything is green, I'll be happy to merge.
| target_chunks, | ||
| } | ||
| proptest::prop_compose! { | ||
| /// Generate a pseudorandom removal record from the given seed, for testing purposes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this comment is out of date.
And now that we are discussing comments on this struct anyway, how about adding a comment about the limitations of this strategy? Specifically, the chunk indices are independent of the absolute index set. This feature might cause some tests to fail if they assume these fields are in sync. I realize the original code had the same limitation and I do not think fixing it is within the scope of this PR -- but adding a comment to disclaim the limitation, is in scope ;-)
There are few ways to formulate the answers which boils down to the very end of #543 (review) line of discussion. I mean I guess they could be converted but what's the point if the decision is to keep the unstructured tests random. |
|
Unfortunately, the test suite is not passing. The following tests are stalling. |
|
I took the random one (`block_with_wrong_mmra_is_invalid`) to have a
look, and I see that it plainly wasn't affected by this PR.
…On Wed, Apr 30 2025 at 08:36:08 AM -0700, aszepieniec ***@***.***> wrote:
*aszepieniec*
left a comment
(Neptune-Crypto/neptune-core#543)
<#543 (comment)>
Unfortunately, the test suite is not passing. The following tests are
stalling.
SIGINT [6065.149s] neptune-cash
peer_loop::peer_loop_tests::block_without_valid_pow_test
SIGINT [6065.536s] neptune-cash
peer_loop::peer_loop_tests::block_request_batch_in_order_test
SIGINT [6073.161s] neptune-cash
models::blockchain::block::block_tests::block_with_wrong_mmra_is_invalid
SIGINT [6072.171s] neptune-cash
models::peer::tests::sync_challenge_response_pow_witnesses_must_be_a_chain
SIGINT [6063.567s] neptune-cash
peer_loop::peer_loop_tests::test_peer_loop_receival_of_third_block_no_blocks_in_db
SIGINT [6071.536s] neptune-cash
models::state::global_state_tests::mock_global_state_is_valid
SIGINT [6063.592s] neptune-cash
peer_loop::peer_loop_tests::test_peer_loop_receival_of_fourth_block_one_block_in_db
SIGINT [6072.079s] neptune-cash
models::state::archival_state::archival_state_tests::block_hash_witness::stored_block_hash_witness_agrees_with_block_hash
SIGINT [6073.167s] neptune-cash
models::blockchain::block::block_tests::block_is_valid::blocks_with_0_to_10_inputs_and_successors_are_valid
SIGINT [6065.097s] neptune-cash
peer_loop::peer_loop_tests::different_genesis_test
SIGINT [6063.905s] neptune-cash
peer_loop::peer_loop_tests::test_peer_loop_block_with_block_in_db
SIGINT [6065.314s] neptune-cash
peer_loop::peer_loop_tests::block_request_batch_out_of_order_test
SIGINT [6072.178s] neptune-cash
models::peer::transfer_block::test::from_transfer_block
SIGINT [6065.072s] neptune-cash
peer_loop::peer_loop_tests::find_canonical_chain_when_multiple_blocks_at_same_height_test
SIGINT [6063.596s] neptune-cash
peer_loop::peer_loop_tests::test_peer_loop_receival_of_second_block_no_blocks_in_db
SIGINT [6073.188s] neptune-cash
models::blockchain::block::block_tests::block_is_valid::block_with_far_future_timestamp_is_invalid
SIGINT [6065.570s] neptune-cash
peer_loop::peer_loop_tests::block_proposals::accept_block_proposal_height_one
SIGINT [6064.610s] neptune-cash
peer_loop::peer_loop_tests::receive_block_request_by_height_block_7
SIGINT [6064.766s] neptune-cash
peer_loop::peer_loop_tests::receival_of_block_notification_height_1
SIGINT [6063.924s] neptune-cash
peer_loop::peer_loop_tests::test_block_reconciliation_interrupted_by_peer_list_request
SIGINT [6064.188s] neptune-cash
peer_loop::peer_loop_tests::sync_challenges::bad_sync_challenge_height_greater_than_tip
SIGINT [6064.039s] neptune-cash
peer_loop::peer_loop_tests::test_block_reconciliation_interrupted_by_block_notification
SIGINT [6063.611s] neptune-cash
peer_loop::peer_loop_tests::test_peer_loop_receival_of_first_block
SIGINT [6064.890s] neptune-cash
peer_loop::peer_loop_tests::prevent_ram_exhaustion_test
SIGINT [6065.570s] neptune-cash
peer_loop::peer_loop_tests::block_proposals::accept_block_proposal_notification_height_one
SIGINT [6065.275s] neptune-cash
peer_loop::peer_loop_tests::block_request_batch_simple
SIGINT [6064.163s] neptune-cash
peer_loop::peer_loop_tests::sync_challenges::sync_challenge_happy_path
—
Reply to this email directly, view it on GitHub
<#543 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/APXLOT4JBCYQKSV3W6TF7DT24DUWRAVCNFSM6AAAAAB2LEUFPSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQNBSGQYDENRQGY>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Okay. In this case I am profoundly confused by why the test behaves differently on master from on this branch, as observed by my machine: Did you observe zero (or rather: within error margin) time disparity for this test between the two branches? |
|
I'll try that on `master` too after the hackathon. IIRC I always
observed tests working better on `master`, but after you noticed that
there's cache for test cases I turned my eye blind to the difference.
🤷♂️
…On 2025-05-02 13:02, aszepieniec wrote:
aszepieniec left a comment (Neptune-Crypto/neptune-core#543) [1]
> I took the random one (block_with_wrong_mmra_is_invalid) to have a
> look, and I see that it plainly wasn't affected by this PR.
Okay. In this case I am profoundly confused by why the test behaves
differently on master from on this branch, as observed by my machine:
|
At risk of stating the obvious, the test is most likely slow because it does not find a cached proof and is attempting to generate a new proof. So this implies that the code on the branch is doing something different in a way that causes proof inputs (program, claim, nondeterminism) to be different somehow. |
I noticed some of those from your list was quite fast; overall is: "test result: ok. 646 passed; 0 failed; 2 ignored; 0 measured; 0 filtered out; finished in 197.28s". |
|
Sorry for the miscommunication. I thought the ball was in your court. I've been running I'll leave the test running for a few more hours, just to make sure. But so far it looks as though the stall is not caused by absent proofs. So I would say the ball is back in your court not to figure out what is causing it. If you are running into the problem of absent proofs, you can tell because you will start producing the proofs (or trying to). Unless you are running the test on a number cruncher, that task will eat all your RAM and CPU cores and will probably even be killed by the operating system. |
|
I ran the test suite. All tests pass, except for one: The behavior of this test is confusing. On the number cruncher that produced the proofs, it fails immediately. On my desktop machine, after lifting the proofs, the test runs seemingly forever and consumes negligible resources while doing so. I suspect that proptest is at play: it found a failing instance on the number cruncher and is running the test on that instance first thing every time. So forgive me for a rather lengthy error message as it includes the failing instance. I used the opportunity to copy the proofs to one of the proof servers which serves them now. If all is well you should be able to query them and that should happen automatically under the hood when you run tests that require proofs. If proofs are needed after the point where the failure occurs then those have not been generated yet, but as it is a proptest I don't think there are any. So in summary, you should have access to all proofs you need now. (And if you don't -- please poke me.) |
|
(It's absolutely cool GH allow these long texts, but I suggest to move the output to a pastebin just for the scrolling comfort. Otherwise no problem with that.) So, after the quick look the only I wonder what would you think of adding a type like |
Noted for next time. The root cause of the issue is that After that refactor, we modified the test to filter out blocks with negative transaction fees. The test in question passes now, along with all the other ones. We also tried to merge into master but the resulting merge conflict led to head-scratching and guesswork and we think it's better if we pass the ball back to you because you have context readily at hand that we don't. So -- please rebase on top of master and if everything is still green then we'll merge right away. Sorry for the very drawn-out review process. If it's any consolation, we are very happy with this PR. |
return the <.gitignore> change Revert "initial approach" This reverts commit f5f4b66. the second approach impacting a cached/deterministic/derandomized test align with the review comments replace `TestRunner` with some proper invocations replacing more `TestRunner` couple of strategies on `ChunkDictionary` `TransactionKernel` strategies `prop_compose!` `Utxo` add `Seed` and ditch `rng` invocations from block faking helpers it isn't meaningful to fuzz randomness `tests::shared` doesn't generate it's own randomness anymore last place of randomness in `tests::shared` ditched semantically finished ready for review ditch #proofs_presaved_compat Update src/models/state/wallet/secret_key_material.rs Co-authored-by: aszepieniec <[email protected]> the comment on the `prop_assume` for seeds an import unmerge ditch a temp binding propose solution for `Hash`-based collections in `proptest` expand the proposal to the place where the discussion is changes are moved into Neptune-Crypto#554 finilize `catch_inconsistent_shares` remove few commented out lines consistent naming reflect in the name that "the chunk indices are independent of the absolute index set" fix staling of some tests
8013e82 to
7cfa8a1
Compare
|
Can confirm that all tests pass. So I merged. Thank you for your contribution! There is of course the decoration |
|
🎉 |
Function `Block::total_guesser_reward`, along with other helper methods related to guesser fees, are undefined for un-validated blocks, so the result should be wrapped in a `Result` in order to avoid passing inconsistent values up the call graph. This change cascades into many function return type modifications and many `.unwrap`s and `.expect`s downstream. The inconsistency was exposed by test `rpc_server::rpc_server_tests::public_announcements_in_block_test` which is now fixed instead of ignored. Thanks to @skaunov for PR #543 and the effort leading to this exposition. Also: - Change logic for computing block proposal favorability: use `prev_block_digest` instead of block height. However, the old method (now suffixed with `_legacy`) continues to exist to enable unmodified processing of `PeerMessage` `BlockProposalNotification`, which comes with a block height and not a digest. (We cannot change `PeerMessage`, unfortunately.) - Rewrite handler for `PeerMessage` `BlockProposal`. Simplify and reduce indentation. Co-authored-by: Alan Szepieniec <[email protected]> Co-authored-by: Thorkil Schmidiger <[email protected]>
Function `Block::total_guesser_reward`, along with other helper methods related to guesser fees, are undefined for un-validated blocks, so the result should be wrapped in a `Result` in order to avoid passing inconsistent values up the call graph. This change cascades into many function return type modifications and many `.unwrap`s and `.expect`s downstream. The inconsistency was exposed by test `rpc_server::rpc_server_tests::public_announcements_in_block_test` which is now fixed instead of ignored. Thanks to @skaunov for PR #543 and the effort leading to this exposition. Also: - Change logic for computing block proposal favorability: use `prev_block_digest` instead of block height. However, the old method (now suffixed with `_legacy`) continues to exist to enable unmodified processing of `PeerMessage` `BlockProposalNotification`, which comes with a block height and not a digest. (We cannot change `PeerMessage`, unfortunately.) - Rewrite handler for `PeerMessage` `BlockProposal`. Simplify and reduce indentation. Co-authored-by: Alan Szepieniec <[email protected]> Co-authored-by: Thorkil Schmidiger <[email protected]>
Function `Block::total_guesser_reward`, along with other helper methods related to guesser fees, are undefined for un-validated blocks, so the result should be wrapped in a `Result` in order to avoid passing inconsistent values up the call graph. This change cascades into many function return type modifications and many `.unwrap`s and `.expect`s downstream. The inconsistency was exposed by test `rpc_server::rpc_server_tests::public_announcements_in_block_test` which is now fixed instead of ignored. Thanks to @skaunov for PR #543 and the effort leading to this exposition. Also: - Change logic for computing block proposal favorability: use `prev_block_digest` instead of block height. However, the old method (now suffixed with `_legacy`) continues to exist to enable unmodified processing of `PeerMessage` `BlockProposalNotification`, which comes with a block height and not a digest. (We cannot change `PeerMessage`, unfortunately.) - Rewrite handler for `PeerMessage` `BlockProposal`. Simplify and reduce indentation. Co-authored-by: Alan Szepieniec <[email protected]> Co-authored-by: Thorkil Schmidiger <[email protected]>
* refactor!: Wrap fallible guesser fee helper functions in `Result` Function `Block::total_guesser_reward`, along with other helper methods related to guesser fees, are undefined for un-validated blocks, so the result should be wrapped in a `Result` in order to avoid passing inconsistent values up the call graph. This change cascades into many function return type modifications and many `.unwrap`s and `.expect`s downstream. The inconsistency was exposed by test `rpc_server::rpc_server_tests::public_announcements_in_block_test` which is now fixed instead of ignored. Thanks to @skaunov for PR #543 and the effort leading to this exposition. Also: - Change logic for computing block proposal favorability: use `prev_block_digest` instead of block height. However, the old method (now suffixed with `_legacy`) continues to exist to enable unmodified processing of `PeerMessage` `BlockProposalNotification`, which comes with a block height and not a digest. (We cannot change `PeerMessage`, unfortunately.) - Rewrite handler for `PeerMessage` `BlockProposal`. Simplify and reduce indentation. * chore: Gracefully catch errors resulting from invalid blocks The peer loop should act as a robust firewall against invalid blocks. Nevertheless, it is worth ensuring that handlers behind that firewall cannot crash even for invalid blocks. This commit replaces `.unwrap()`s and `.expect(..)`s with question marks, and wraps the return type of some handlers in a `Result` where necessary. Thanks to @skaunov for identifying the issues. * docs(Block): Adjust a few error messages related to blocks * docs: Adjust some error messages related to negative tx-fees in block Adjust messages to avoid implying that blocks stored in state must be valid, since that's not true for tests. --------- Co-authored-by: Alan Szepieniec <[email protected]> Co-authored-by: sword_smith <[email protected]>
(resolve #110)
Tests which require no structure in their input and intake only crypto randomness have no chance to have an advantage switching to
proptestas it kind of searching an input space which feels pointless in such a case but bring overhead. Boundary tests seems to be a better thing (like trying identity elements, order, prime, maximal).I've started trying to convert like <src/main_loop/proof_upgrader.rs> or <src/models/state/archival_state.rs> (<src/models/state/mod.rs>, <src/models/state/wallet/wallet_state.rs>), but just adapting it to helpers derandomized fits better. (The first approach was an attempt to do everything with a
TestRunnerdrilling it down a call stack.)helpers randomness
I feel that helpers should not generate more/own randomness as they can be reused in a
proptestsetting and only inspection of their calls can reveal if they do. =( I addedRandomness(wasSeeds) to pass a number of random values without cluttering arguments. Approach withpophelps to find mistakes in a test.an awkward arbitrary test
block_hash_relates_to_predecessor_difficultyreceived a bit awkwardarbitrarytest instead of just random due to it's the only place usingrandom_transaction_kernelhelper and making no sense to be converted in aproptestshamirstrategiesI was considering to make a recursive strategy for
shamir, but managed to go without that.Unsurprisingly a straightforward approach to generate a
HashSethits the local rejection limit,#[strategy(proptest::collection::hash_set(0usize..#n, #t))] indices: HashSet<usize>,so I relied on the arbitrariness of the
HashSetitself. Also I've used that approach when modifyingremoval_record_missing_chunk_element_is_invalid_pbt. #arbitraryHashSetIteratorAlso it could be an optimization to
use lazy_staticfor the initial set generation, as it seems to me making aconstfor the initial set isn't possible. Note that it will significantly decrease the arbitrariness of theHashSet.property coverage
For
amount_scalar_mul_pbta naturalStrategyends up in the rejection limit. I chose to go with an approach which better describes the properties of the method and tests the whole domain for the price of hitting the interesting part quite rarely. But I feel it's still often enough, and also a case is added to theproptestfile. There still an option to generate the values for it via division.