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

handholding for tmpl data structures #4001

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

two-heart
Copy link
Collaborator

Add debug time assertions to be used as an early bug oracle during fuzzing and other testing

  • Only based on local information, so handholding can be used without any changes
  • Focus on data structures currently in use Drive-by: typo fixes, make magic unique, mark unreachable code with unreachable builtin

@two-heart two-heart marked this pull request as draft January 23, 2025 11:47
@two-heart two-heart marked this pull request as ready for review January 23, 2025 15:40
nlgripto
nlgripto previously approved these changes Jan 23, 2025
@mmcgee-jump
Copy link
Contributor

I feel like if you're going to do this, you should make it more strict? Basically do a tmpl_verify after every operation to check all invariants

@two-heart
Copy link
Collaborator Author

@mmcgee-jump I updated the PR to call add calls verify wherever:
(1) a verify function is available
(2) the necessary arguments are locally available
(3) the function modifies the data structure
(4) is not a wrapper around a function that already has equivalent checks

For cases that are not checked because of (2) my intention is to do a separate PR that adds verify calls from the usage side at the end of fuzz tests and other testing.

@two-heart
Copy link
Collaborator Author

(rebased on main and resolved conflicts with recent fd_map_chain changes)

@two-heart
Copy link
Collaborator Author

Having handholding in fd_map_giant is not practical because pretty soon the very frequently happening inserts/remove will have an overhead of 8-9s just for running verify. This is way too long to run things like ledger tests.

nlgripto
nlgripto previously approved these changes Jan 24, 2025
@two-heart
Copy link
Collaborator Author

Adjusted the vote program to use the correct wrapping variants of deque push, the previous version might seem to work correctly but leaves the deque data structure in an invalid state.

@nlgripto nlgripto requested a review from mmcgee-jump February 3, 2025 16:24
@two-heart two-heart force-pushed the tmpl-handholding branch 3 times, most recently from 2750377 to ea70336 Compare February 12, 2025 14:37
@two-heart
Copy link
Collaborator Author

Following Kevin's comments, I included logging services only when handholding is enabled in places where we don't do other logging. (+ removed trailing whitespace at unrelated code to make the new gh action check happy)

@two-heart
Copy link
Collaborator Author

Rebased onto main to resolve map_chain merge conflict.

Add debug time assertions to be used as an early bug oracle during fuzzing and other testing
* Only based on local information, so handholding can be used without any changes
* Focus on data structures currently in use
Drive-by: typo fixes, make magic unique, mark unreachable code with unreachable builtin
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.

3 participants