refactor: remove SimpleMemoryStorage and refactor DAA#1026
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1026 +/- ##
==========================================
- Coverage 84.92% 84.83% -0.09%
==========================================
Files 298 297 -1
Lines 22962 22910 -52
Branches 3470 3464 -6
==========================================
- Hits 19500 19436 -64
- Misses 2773 2785 +12
Partials 689 689 ☔ View full report in Codecov by Sentry. |
jansegre
left a comment
There was a problem hiding this comment.
Looks good to me, but I'm interested in how you noticed the performance issue, maybe we could have an automated way to track the performance realistically.
@jansegre I agree! In this case I noticed it pretty much manually during my work on Multiprocess Verification, as it's a performance project so I was comparing syncs multiple times, and then I found this issue. I've been using the hyperfine command-line benchmarking tool, which is great. We could use it in a CI step, I also had mentioned this with Marcelo, especially considering the Multiprocess Verification project. |
Motivation
In previous PRs, in order to implement Multiprocess Verification, we were going to use a class abstraction over the
TransactionStorage, aSimpleMemoryStorage. This was becoming convoluted in later PRs, especially considering Sync-V2 agents will also have their own local memory storages.The abstraction had to include several methods to comply with all callers, however each caller only used a few of them. In other words, the abstraction was too heavy and unnecessary. Therefore, we cancelled that approach and went with a functional abstraction instead. This PR applies that refactor to DAA. Its methods are are now higher-order functions, accepting functional parameters to perform their calculations, instead of a storage. This abstraction is way lighter and more flexible, as now methods explicitly require specific functions, not a bloated class, and callers can provide those functions from whichever object they need.
This PR essentially reverts PR #895, and was initially implemented as part of the Multiprocess Verification project in PR #1022. This change was advanced to now because it undoes significant performance losses introduced by
clone()calls in theSimpleMemoryStorage, that is not going to be used anymore — the sync process will become much quicker, as it was before #895. The removal of the storage dependency from the block verification process is kept in #1022.Acceptance Criteria
SimpleMemoryStorage.VertexVerifier.Checklist
master, confirm this code is production-ready and can be included in future releases as soon as it gets merged