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 more checks for impossible constraints #135

Merged
merged 1 commit into from
Feb 6, 2023
Merged

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented Feb 1, 2023

If a single spend bundle has an ASSERT_BEFORE_HEIGHT_ABSOLUTE that's lower than or equal to another ASSERT_HEIGHT_ABSOLUTE, the spend bundle will never be valid.

This patch checks for cases like this. Also for _SECONDS_ABSOLUTE.

The _RELATIVE checks can only be made within the same Spend, since we don't actually know any absolute timestamps or confirmation heights for the coins.

This is to catch a few obvious invalid cases early.

This still leaves non-obvious cases as successful (and must fail at a later state in the mempool pipeline).

For example, two different coins can have ASSERT_HEIGHT_RELATIVE and ASSERT_BEFORE_HEIGHT_RELATIVE that turn out to conflict, but we'll only know that once we look those coins up from the coin DB and know their confirmation heights.

Another, more subtle, case of invalid spend bundles is where an ASSERT_SECONDS_ABSOLUTE conflicts with an ASSERT_BEFORE_HEIGHT_ABSOLUTE. In chia_rs we don't have any mapping between block height and (predicted) timestamps. Clearly we couldn't reject spend bundles based on predictions anyway, but there may be cases where a SECONDS timestamp is known to refer to an old, existing, block. We won't catch those cases here either.

… combined with assert before height|seconds. Just to catch a few cases earlier
@arvidn arvidn force-pushed the more-assert-before-checks branch from 9c7a633 to 1b0c7e8 Compare February 1, 2023 21:23
@arvidn arvidn marked this pull request as ready for review February 2, 2023 10:13
Copy link
Contributor

@richardkiss richardkiss left a comment

Choose a reason for hiding this comment

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

Seems reasonable. The repeated code feels a bit wrong, but I suppose in this case it's okay.

Alternative is we could just check every time through the loop, or just once at the end of conditions processing. I don't see a case where exiting early buys us much.

@arvidn
Copy link
Contributor Author

arvidn commented Feb 6, 2023

Alternative is we could just check every time through the loop, or just once at the end of conditions processing. I don't see a case where exiting early buys us much.

Yeah, that was my first version, but it seemed hard to justify checking every time

@arvidn arvidn merged commit 24a0972 into main Feb 6, 2023
@arvidn arvidn deleted the more-assert-before-checks branch February 6, 2023 20:29
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