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 full block data for testing and benchmarking #263

Merged
merged 1 commit into from
Oct 29, 2024

Conversation

JoseSK999
Copy link
Contributor

@JoseSK999 JoseSK999 commented Oct 25, 2024

  • Added the block 866,342 to testdata, as well as the spent UTXOs
  • get_validation_flags doesn't need to ask the chainstore for the block hash
  • Changed the name of the inner validate_block to validate_block_no_acc (this way we can clearly distinguish it from BlockchainInterface::validate_block)

This test data provides fast and meaningful insights into block validation performance and correctness. The block includes a transaction with a 60kb inscription.

UPDATE: I have compressed the two files with zstd, reducing the size of the raw block from 1.6MB to 1MB, and the size of the spent UTXOs file from 800KB to just 80KB. The diff is now clean, only reflecting the fairly small code change.

This relates to #260 and #262.

@JoseSK999
Copy link
Contributor Author

Note I didn't use include_bytes! nor include_str! to not have this large data included in the test binary.

@Davidson-Souza
Copy link
Collaborator

This is very interesting. One block that might be useful to test too: mempool.space. It contains one transaction with 19k inputs. Super expensive to check

@JoseSK999
Copy link
Contributor Author

Added! I have made the test for block 367,891 to be ignored in debug mode since it takes way too long. On release mode it is more acceptable (<2 min).

Also the benchmark takes a lot of time because we need to get at least 10 samples of execution. I thought the best way to deal with this is by skipping this bench by default when running cargo bench. Nonetheless it can be included by running EXPENSIVE_BENCHES=1 cargo bench (in my case it takes 18 mins).

@jaoleal
Copy link
Contributor

jaoleal commented Oct 28, 2024

Added! I have made the test for block 367,891 to be ignored in debug mode since it takes way too long. On release mode it is more acceptable (<2 min).

Also the benchmark takes a lot of time because we need to get at least 10 samples of execution. I thought the best way to deal with this is by skipping this bench by default when running cargo bench. Nonetheless it can be included by running EXPENSIVE_BENCHES=1 cargo bench (in my case it takes 18 mins).

I dont think thats bad enough to take out from the default benchmark battery... its not like we are going to use this every time that we develop anything.

Maybe we could trigger this every commit to master and update the results on a session of the README.md.

@JoseSK999
Copy link
Contributor Author

JoseSK999 commented Oct 28, 2024

So the rationale is that if you’re benchmarking different versions of your code, you usually want quick feedback. Keeping a >10 min bench in the default run can really add up and may not even be relevant for a general performance picture (especially given this is a corner case). And if it is important, a dev can just set EXPENSIVE_BENCHES=1, so this approach keeps things efficient without limiting anyone.

@jaoleal
Copy link
Contributor

jaoleal commented Oct 28, 2024

as you wish, broda.

@Davidson-Souza
Copy link
Collaborator

LGTM.

Added! I have made the test for block 367,891 to be ignored in debug mode since it takes way too long. On release mode it is more acceptable (<2 min).
Also the benchmark takes a lot of time because we need to get at least 10 samples of execution. I thought the best way to deal with this is by skipping this bench by default when running cargo bench. Nonetheless it can be included by running EXPENSIVE_BENCHES=1 cargo bench (in my case it takes 18 mins).

I think it would be nice to have this documented, likely in the readme.

- Added the blocks 866,342 and 367,891 to `testdata`, as well as the spent UTXOs in each block
- `get_validation_flags` doesn't need to ask the chainstore for the block hash
- Changed the name of the inner `validate_block` to `validate_block_no_acc`
- Update README.md
@JoseSK999
Copy link
Contributor Author

Rebased and updated README.md (second push is because I forgot to update the table of contents)

@Davidson-Souza
Copy link
Collaborator

ACK ac37630

@Davidson-Souza Davidson-Souza merged commit cd939ec into vinteumorg:master Oct 29, 2024
6 checks passed
@JoseSK999 JoseSK999 deleted the block-testdata branch October 29, 2024 17:41
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