feat(consensus): Add new verifications for mempool only#1379
Conversation
80f6f7f to
6d0cfa0
Compare
|
| Branch | feat/mempool-verifications |
| Testbed | ubuntu-22.04 |
🚨 1 Alert
| Benchmark | Measure Units | View | Benchmark Result (Result Δ%) | Upper Boundary (Limit %) |
|---|---|---|---|---|
| sync-v2 (up to 20000 blocks) | Latency minutes (m) | 📈 plot 🚷 threshold 🚨 alert (🔔) | 2.27 m(+32.98%)Baseline: 1.71 m | 2.05 m (110.81%) |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result minutes (m) (Result Δ%) | Lower Boundary minutes (m) (Limit %) | Upper Boundary minutes (m) (Limit %) |
|---|---|---|---|---|
| sync-v2 (up to 20000 blocks) | 📈 view plot 🚷 view threshold 🚨 view alert (🔔) | 2.27 m(+32.98%)Baseline: 1.71 m | 1.54 m (67.68%) | 2.05 m (110.81%) |
6d0cfa0 to
5222b85
Compare
1930e49 to
3817185
Compare
0741c79 to
8d563c0
Compare
| if params.reject_too_old_vertices: | ||
| self.verifiers.vertex.verify_old_timestamp(vertex) |
There was a problem hiding this comment.
I think I prefer having the conditionals inside the methods. That is, just call them here and they'll be nops depending on the params.
The reason being, I think the verification service should just be a "dumb router", and it would be the method's responsibility to know when it should or not make the check, which makes sense because being mempool-only is something specific and internal for each method. Also, it makes sure that the correct conditionals are used whenever these methods are called, even if they're called outside the verification service.
Same for all other calls. What do you think?
There was a problem hiding this comment.
I guess this solution is okay for now, but I think we should refactor it in the future to create a better separation of mempool-only checks and existing verification. What do you think?
There was a problem hiding this comment.
Should we add all new mempool-only verifications to the consensus handling of reorgs, to remove invalid txs when the mempool changes?
There was a problem hiding this comment.
Yes, I'll do it in another PR.
There was a problem hiding this comment.
Should we add all new verification methods to test_verification.py?
There was a problem hiding this comment.
Yes, I'll do it in another PR.
| for cp in self._settings.CHECKPOINTS: | ||
| if cp.height == block_height and cp.hash != block.hash: | ||
| raise CheckpointError(f'invalid checkpoint block (height={cp.height})') |
There was a problem hiding this comment.
Should we refactor the settings.CHECKPOINTS attribute to be a dict with height keys instead of a list, so we can replace this O(n) with O(1)? At the very least we could break out of the loop when the height is found.
There was a problem hiding this comment.
I reduced the O(n) to O(1) but without changing the settings.CHECKPOINTS type.
|
|
||
| nano_header = tx.get_nano_header() | ||
| if nano_header.is_creating_contract(): | ||
| # TODO Confirm that the blueprint exists. |
|
|
||
| def verify_standard_scripts(self, tx: Transaction) -> None: | ||
| """Verify that all outputs have standard scripts.""" | ||
| pass |
There was a problem hiding this comment.
I decided to not add this verification now. I'll do it separately in the consensus simplify project.
8d563c0 to
897f6b4
Compare
| block_storage = self._tx_storage.get_nc_block_storage(best_block) | ||
| seqnum = block_storage.get_address_seqnum(nano_header.nc_address) | ||
| diff = nano_header.nc_seqnum - seqnum | ||
| if diff < 0 or diff > MAX_SEQNUM_JUMP_SIZE: |
There was a problem hiding this comment.
This might prevent the same address from having "a lot of nano transactions" in the same block.
| def verify_seqnum(self, tx: BaseTransaction) -> None: | ||
| assert tx.is_nano_contract() | ||
| assert isinstance(tx, Transaction) | ||
|
|
||
| nano_header = tx.get_nano_header() | ||
| best_block = self._tx_storage.get_best_block() | ||
| block_storage = self._tx_storage.get_nc_block_storage(best_block) | ||
| seqnum = block_storage.get_address_seqnum(nano_header.nc_address) | ||
| diff = nano_header.nc_seqnum - seqnum | ||
| if diff < 0 or diff > MAX_SEQNUM_JUMP_SIZE: | ||
| raise NCInvalidSeqnum(f'invalid seqnum (diff={diff})') |
There was a problem hiding this comment.
Should we iterate over all txs in the mempool instead of getting the best block storage?
897f6b4 to
3c9f5ed
Compare
17a16a8 to
7fc61e7
Compare
fa7fd1d to
52dcc09
Compare
39aca90 to
62447bf
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1379 +/- ##
==========================================
+ Coverage 85.74% 85.83% +0.08%
==========================================
Files 431 431
Lines 33133 33278 +145
Branches 5196 5225 +29
==========================================
+ Hits 28411 28564 +153
+ Misses 3679 3673 -6
+ Partials 1043 1041 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
74eeb43 to
3088105
Compare
be64c1c to
e84308c
Compare
5031493 to
eec4b15
Compare
eec4b15 to
297959c
Compare
Motivation
Add several verifications for mempool only (but one). As it impacts only the mempool, the policies can be more strict than the blockchain protocol itself.
Acceptance Criteria
BlockVerifier.verify_checkpoints()for all blocks.NanoHeaderVerifier.verify_method_call()for nano transactions entering the mempool. This requires the blueprint to exist, the contract to have already been created, the method to exist (or a fallback method), and the call arguments to be parseable.NanoHeaderVerifier.verify_seqnum()for nano transactions entering the mempool.TransactionVerifier.verify_conflict()for transactions entering the mempool. This forbids transactions in the mempool to have a conflict with an already confirmed transaction.TransactionVerifier.verify_token()for transactions entering the mempool. This limits the number of tokens, ensure that there is no duplicate token in the list, and ensure that all tokens are used at least once.VertexVerifier.verify_old_timestamp()for all vertices entering the mempool. This limits how old these vertices can be.Checklist
master, confirm this code is production-ready and can be included in future releases as soon as it gets merged