-
Notifications
You must be signed in to change notification settings - Fork 145
Store union of hashes for block witness #2109
Conversation
613711a
to
1c2d53c
Compare
Sorry, I keep forgetting to review this. Will do so tomorrow, but just thought I'd mention I found a bug that causes Is that bug present in this branch as well? |
Yup, looks like that bug is fixed in this branch. I added the test. It's similar to another test I added, but seems good to have another explicit one. 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
tests/core/p2p-proto/test_wit_db.py
Outdated
""" | ||
Repeated blocks should not consume more slots in the limited history of block witnesses | ||
""" | ||
wit_db = AsyncWitnessDB(AtomicDB()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AsyncWitnessDB was changed recently to wrap a regular DatabaseAPI
instead of AtomicDatabaseAPI
, so we could use a MemoryDB()
here instead
Atomic is not called for, anymore
Repeated persistence with the same block should not consume history slots. I tested the test by patching into line 59 of trinity/protocol/wit/db.py: + if True or block_hash not in recent_blocks_with_witnesses: The test fails with that change, as expected. Also, explicitly check that DB eviction works after a block repeat is persisted.
14b6325
to
653f264
Compare
What was wrong?
Fixes #2105
How was it fixed?
Store union of the witness hashes, when witness metadata comes in for the same block.
To-Do
Test where two persists interleave their database accesses, to force the race condition that one witness overwrites the otherAdd a lock to pass ^ testAdd entry to the release notesCute Animal Picture