fix: Fix rare conditions captured by random simulations #283
Conversation
msbrogli
force-pushed
the
fix/tests-simulator
branch
3 times, most recently
from
20fb9a8 to
568a6ea
Compare
msbrogli
force-pushed
the
fix/tests-simulator
branch
from
568a6ea to
e5dddc4
Compare
Codecov Report
@@ Coverage Diff @@
## dev #283 +/- ##
==========================================
+ Coverage 82.16% 82.21% +0.05%
==========================================
Files 152 152
Lines 14408 14423 +15
Branches 2056 2061 +5
==========================================
+ Hits 11838 11858 +20
Misses 2132 2132
+ Partials 438 433 -5
Continue to review full report at Codecov.
|
| if not self.with_index: | ||
| raise NotImplementedError | ||
| assert self.block_index is not None | ||
| assert self.tx_index is not None | ||
| if self.tokens_index: | ||
| self.tokens_index.del_tx(tx) | ||
| if tx.is_block: | ||
| self._cache_block_count -= 1 | ||
| self.block_index.del_tx(tx, relax_assert=relax_assert) | ||
| # This is the code to remove the block from the indexes. |
There was a problem hiding this comment.
I think it might get confusing here when we should set del_blocks=True. If I understood correctly, only when you want to completely remove the block from the storage is when you should set it, right? Or is there any other situation? Maybe we could have this as a comment just to prevent mistakes
I am saying that because on the consensus file we are not using del_blocks=True and on remove_transaction we are. Instead of re-reading all places to understand the reasoning behind, we could just write here.
There was a problem hiding this comment.
I don't know if this information adds anything: as far as I know there is only one case in practice where a fullnode will remove a transaction by itself (that is without you opening a shell and doing it manually), it is when there is a re-org that changes the reward-lock validity of a transaction. In all other cases the uses of remove_transaction are artificial which are basically used in tests.
| @@ -59,14 +59,16 @@ def add_tx(self, tx: BaseTransaction) -> bool: | |||
| assert r1 == r2 | |||
| return r1 | |||
|
|
|||
| def del_tx(self, tx: BaseTransaction, *, relax_assert: bool = False) -> None: | |||
| def del_tx(self, tx: BaseTransaction, *, relax_assert: bool = False) -> bool: | |||
| """ Delete a transaction from the indexes | |||
|
|
|||
| :param tx: Transaction to be deleted | |||
| """ | |||
There was a problem hiding this comment.
Update docstring with meaning the return value.
| @@ -161,15 +163,15 @@ def add_tx(self, tx: BaseTransaction) -> bool: | |||
| self.tx_last_interval[tx.hash] = interval | |||
| return True | |||
|
|
|||
| def del_tx(self, tx: BaseTransaction, *, relax_assert: bool = False) -> None: | |||
| def del_tx(self, tx: BaseTransaction, *, relax_assert: bool = False) -> bool: | |||
| """ Remove a transaction from the index. | |||
| """ | |||
There was a problem hiding this comment.
Update docstring with meaning the return value.
| @@ -293,14 +297,16 @@ def add_tx(self, tx: BaseTransaction) -> bool: | |||
| self.transactions.add(element) | |||
| return True | |||
|
|
|||
| def del_tx(self, tx: BaseTransaction) -> None: | |||
| def del_tx(self, tx: BaseTransaction) -> bool: | |||
| """ Delete a transaction from the index | |||
|
|
|||
| :param tx: Transaction to be deleted | |||
| """ | |||
There was a problem hiding this comment.
Update docstring with meaning the return value.
| # without going through the whole download. | ||
| for deferred in pending: | ||
| yield deferred | ||
| pending = WeakSet() |
There was a problem hiding this comment.
Isn't it slightly better to do pending.clear() to avoid an allocation and having to wait for the GC to collect the previous set?
| # block rewards. | ||
| if del_blocks: | ||
| if self.block_index.del_tx(tx, relax_assert=relax_assert): | ||
| self._cache_block_count -= 1 |
There was a problem hiding this comment.
Are we still using tx_storage.tx_index, tx_storage.block_index or tx_storage.all_index? I just checked and apparently there's only some leftover references from before the last refactor, and they use hasattr, so the code wouldn't even be running currently. Is this really the case?
No description provided.