Skip to content

Conversation

@arvidn
Copy link
Contributor

@arvidn arvidn commented Nov 27, 2025

Purpose:

Use built-in function filter() instead of raw loop, checking and counting in get_seed_for_proof(). Add asserts and checks to validate my understanding of this code.

@arvidn arvidn added Changed Required label for PR that categorizes merge commit message as "Changed" for changelog Exclude_Notes Use this label if the changes in the PR should be excluded from the release notes labels Nov 27, 2025
@arvidn arvidn marked this pull request as ready for review November 27, 2025 01:22
@arvidn arvidn requested a review from a team as a code owner November 27, 2025 01:22
@arvidn arvidn requested a review from almogdepaz November 27, 2025 01:22

summary_heights = self.blockchain.get_ses_heights()
if len(summary_heights) <= 1:
log.error("failed not enough sub epochs")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_seed_for_proof expects at least two sub epochs, it will throw without this.
we cannot get a sees if we dont have 2 finished sub epochs at least

we cannot change how the seed is chosen since it is verified by the verifier so we can change this without being incompatible

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean by this comment. It is possible to call this function with a height that only yields a single summary height. The existing behavior is to throw a KeyError exception in this case. It took some investigation to understand what that meant. I would think this is an improvement, as a clearer error is printed to the log. I'm pretty confident returning None has essentially the same effect as the exception, we fail to respond to this weight proof request either way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this error should be thrown from inside get_seed_for_proof

i missed that this is supposed to be equivalent to the current code (i think its less readable but maybe im just familiar with the old) i always prefer not fixing something thats not broken especially critical code, we can add the error there instead of the assert thats fine

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, it's a good point that it's always risky to change code that works. I think this especially applies to get_seed_for_proof(). I'll propose a less intrusive change to that function.

Regarding the error reporting here. Is this case materially different from the other failure cases here, to warrant an exception instead of returning None?

For example, if self.blockchain.try_block_record(tip) or self._get_recent_chain(tip_rec.height) returns None.

@arvidn arvidn marked this pull request as draft December 2, 2025 00:42
@arvidn arvidn closed this Dec 11, 2025
@arvidn arvidn deleted the simplify-weight-proof branch December 11, 2025 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Changed Required label for PR that categorizes merge commit message as "Changed" for changelog Exclude_Notes Use this label if the changes in the PR should be excluded from the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants