Skip to content

refactor(verification): externalize verification dependencies [part 2/2]#904

Closed
glevco wants to merge 1 commit intorefactor/verification-dependenciesfrom
refactor/verification-dependencies-part-2
Closed

refactor(verification): externalize verification dependencies [part 2/2]#904
glevco wants to merge 1 commit intorefactor/verification-dependenciesfrom
refactor/verification-dependencies-part-2

Conversation

@glevco
Copy link
Contributor

@glevco glevco commented Dec 22, 2023

Depends on #859

Motivation

Finish the refactor started in #859 (read its description for more info). After this PR, all verification dependencies are externalized and pre-calculated.

Acceptance Criteria

  • Add missing verification dependencies for transactions: token info and best block tips.
  • Add new auxiliary methods to SimpleMemoryStorage.
  • Update verifiers and VerificationService accordingly.
  • Update Transaction methods related to token info to receive an optional SimpleMemoryStorage, which may be used to retrieve data.

Checklist

  • If you are requesting a merge into master, confirm this code is production-ready and can be included in future releases as soon as it gets merged

@glevco glevco self-assigned this Dec 22, 2023
@codecov
Copy link

codecov bot commented Dec 22, 2023

Codecov Report

Attention: Patch coverage is 98.01980% with 4 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (refactor/verification-dependencies@6578961). Click here to learn what that means.

❗ Current head 2bdaa2a differs from pull request most recent head 74b1849. Consider uploading reports for the commit 74b1849 to get more accurate results

Files Patch % Lines
...athor/transaction/storage/simple_memory_storage.py 94.64% 3 Missing ⚠️
hathor/verification/block_verifier.py 90.90% 1 Missing ⚠️
Additional details and impacted files
@@                          Coverage Diff                          @@
##             refactor/verification-dependencies     #904   +/-   ##
=====================================================================
  Coverage                                      ?   85.38%           
=====================================================================
  Files                                         ?      290           
  Lines                                         ?    22508           
  Branches                                      ?     3385           
=====================================================================
  Hits                                          ?    19219           
  Misses                                        ?     2618           
  Partials                                      ?      671           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@glevco glevco changed the title refactor(verification): externalize verification dependencies [part 2] refactor(verification): externalize verification dependencies [part 2/2] Dec 22, 2023
@glevco glevco force-pushed the refactor/verification-dependencies branch 2 times, most recently from 0c9121f to 8b2e539 Compare December 23, 2023 01:59
@glevco glevco force-pushed the refactor/verification-dependencies-part-2 branch 2 times, most recently from fc1e6c0 to 1a93e95 Compare December 23, 2023 02:14
@glevco glevco marked this pull request as ready for review December 23, 2023 02:20
try:
spent_output = spent_tx.outputs[tx_input.index]
except IndexError:
raise InexistentInput(
Copy link
Member

Choose a reason for hiding this comment

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

Have you had this exception raised? I mean, this condition would be impossible if the transaction has been validated.

No problem with this change. Just checking if this condition occurred with you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just that before, this method was only called during verification. Now, it's called before verification (when retrieving the verification dependencies). So, if the tx is invalid, it will raise this exception during that retrieval.

token_dict[token_uid] = TokenInfo(sum_tokens, token_info.can_mint, token_info.can_melt)

def iter_spent_rewards(self) -> Iterator[Block]:
def iter_spent_rewards(self, storage: Optional['SimpleMemoryStorage'] = None) -> Iterator[Block]:
Copy link
Member

Choose a reason for hiding this comment

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

It seems like we are juggling to make the new memory storage work alongside the current code. I think we can spawn an ephemeral verifier that carries a storage and use it to run the verification. It can be either set to tx.storage or to any other storage. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, this was not a good solution. I have solved this using a StorageProtocol, so both TransacionStorage and SimpleMemoryStorage have an unified interface that can be received by those methods. It was implemented in #922, and after it's merged to master I'll rebase this PR so this is updated.

@glevco glevco force-pushed the refactor/verification-dependencies branch from 8b2e539 to 937fc81 Compare December 27, 2023 18:19
@glevco glevco force-pushed the refactor/verification-dependencies-part-2 branch from 1a93e95 to 02bdf05 Compare December 27, 2023 18:20
@glevco glevco force-pushed the refactor/verification-dependencies branch 4 times, most recently from 555be09 to 7e42779 Compare January 9, 2024 21:46
@glevco glevco force-pushed the refactor/verification-dependencies-part-2 branch from 02bdf05 to ddfa87a Compare January 9, 2024 21:53
@glevco glevco force-pushed the refactor/verification-dependencies branch 4 times, most recently from 44842b7 to c8261f4 Compare January 9, 2024 22:44
@glevco glevco force-pushed the refactor/verification-dependencies-part-2 branch from ddfa87a to 33f50f3 Compare January 9, 2024 22:50
@glevco glevco force-pushed the refactor/verification-dependencies branch from c8261f4 to 021a595 Compare January 11, 2024 23:54
@glevco glevco force-pushed the refactor/verification-dependencies-part-2 branch from 33f50f3 to 28be92e Compare January 11, 2024 23:55
@glevco glevco force-pushed the refactor/verification-dependencies branch 2 times, most recently from 1618015 to 2a6d4d0 Compare January 23, 2024 03:04
@glevco glevco force-pushed the refactor/verification-dependencies-part-2 branch from 28be92e to 2bdaa2a Compare January 23, 2024 03:14
@glevco glevco force-pushed the refactor/verification-dependencies branch from 2a6d4d0 to 02efe61 Compare April 1, 2024 16:35
@glevco glevco force-pushed the refactor/verification-dependencies-part-2 branch from 2bdaa2a to eb46aab Compare April 1, 2024 16:37
@glevco glevco force-pushed the refactor/verification-dependencies branch from 02efe61 to 6397eb3 Compare April 3, 2024 23:39
@glevco glevco force-pushed the refactor/verification-dependencies-part-2 branch from eb46aab to 9c3e425 Compare April 3, 2024 23:41
@glevco glevco mentioned this pull request Apr 4, 2024
1 task
@glevco glevco force-pushed the refactor/verification-dependencies branch 2 times, most recently from 91f19c2 to 9e7caf3 Compare April 9, 2024 16:01
@glevco glevco force-pushed the refactor/verification-dependencies-part-2 branch 2 times, most recently from adaaa85 to 792e630 Compare April 12, 2024 18:26
@glevco glevco force-pushed the refactor/verification-dependencies branch from f156d96 to 6b0c206 Compare April 17, 2024 00:37
@glevco glevco force-pushed the refactor/verification-dependencies-part-2 branch from 792e630 to 9e2e668 Compare April 17, 2024 00:49
@glevco glevco force-pushed the refactor/verification-dependencies branch from 6b0c206 to 6578961 Compare May 6, 2024 00:20
@glevco glevco force-pushed the refactor/verification-dependencies-part-2 branch from 9e2e668 to 74b1849 Compare May 6, 2024 00:27
@glevco glevco closed this May 6, 2024
@glevco
Copy link
Contributor Author

glevco commented May 7, 2024

Moved to PRs #1022 and #1023.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants