Skip to content

feat: add safeguards for partially validated transactions [part 2/2]#617

Merged
jansegre merged 1 commit intomasterfrom
feat/validation-safeguards
May 24, 2023
Merged

feat: add safeguards for partially validated transactions [part 2/2]#617
jansegre merged 1 commit intomasterfrom
feat/validation-safeguards

Conversation

@jansegre
Copy link
Member

@jansegre jansegre commented May 18, 2023

This change was split out from #577. The main objective is to be able to do this:

tx.set_validation(ValidationState.BASIC)  # this is a partially validated transaction
tx_storage.save_transaction(tx)  # this fails with an assertion error
with stx_storage.allow_partially_validated_context():
    tx_storage.save_transaction(tx)  # but this succeeds
tx_storage.get_transaction(tx.hash)  # this also fails, but with an error that inherits from "not found"
with stx_storage.allow_partially_validated_context():
    tx_storage.get_transaction(tx.hash)  # but this succeeds

This change is only exists to reduce the scope where the storage APIs accept reading/writing partially validated transactions so that the changes needed by sync-v2 can have a very reduced an limited impact on the rest of the node. The previous iteration of this protection was made using a allow_partially_validated: bool argument on most APIs, that ended up being not only verbose, but more error prone and needed to carry this allow_partially_validated in a lot of places that shouldn't have to know about that.

Acceptance Criteria

  • TransactionStorage must have a scope that dictates which validation level on accept at save/get methods
  • TransactionStorage must have methods that return contextmanagers which temporarily modify that scope

@jansegre jansegre self-assigned this May 18, 2023
@jansegre jansegre requested a review from msbrogli as a code owner May 18, 2023 21:43
@jansegre jansegre force-pushed the feat/validation-safeguards branch from 03f711b to 328e50b Compare May 18, 2023 21:55
@jansegre jansegre force-pushed the feat/validation-safeguards branch from 328e50b to dedf8b5 Compare May 18, 2023 22:44
msbrogli
msbrogli previously approved these changes May 19, 2023
@msbrogli msbrogli force-pushed the feat/partial-validation branch from 87537d7 to 33fb275 Compare May 19, 2023 15:57
Base automatically changed from feat/partial-validation to master May 19, 2023 15:58
@jansegre jansegre force-pushed the feat/validation-safeguards branch 2 times, most recently from a7609b1 to 95713b9 Compare May 19, 2023 20:07
@codecov
Copy link

codecov bot commented May 19, 2023

Codecov Report

Merging #617 (f7b7cd8) into master (17eacb2) will increase coverage by 0.05%.
The diff coverage is 94.23%.

❗ Current head f7b7cd8 differs from pull request most recent head 5005cb6. Consider uploading reports for the commit 5005cb6 to get more accurate results

@@            Coverage Diff             @@
##           master     #617      +/-   ##
==========================================
+ Coverage   83.78%   83.84%   +0.05%     
==========================================
  Files         238      237       -1     
  Lines       19990    20016      +26     
  Branches     2735     2731       -4     
==========================================
+ Hits        16749    16782      +33     
+ Misses       2642     2635       -7     
  Partials      599      599              
Impacted Files Coverage Δ
hathor/manager.py 70.30% <71.42%> (-0.31%) ⬇️
hathor/transaction/base_transaction.py 91.23% <85.71%> (+1.24%) ⬆️
hathor/transaction/storage/tx_allow_scope.py 93.10% <93.10%> (ø)
hathor/consensus/consensus.py 100.00% <100.00%> (ø)
hathor/transaction/storage/cache_storage.py 94.16% <100.00%> (ø)
hathor/transaction/storage/exceptions.py 100.00% <100.00%> (ø)
hathor/transaction/storage/memory_storage.py 91.04% <100.00%> (ø)
hathor/transaction/storage/rocksdb_storage.py 94.77% <100.00%> (+2.07%) ⬆️
hathor/transaction/storage/transaction_storage.py 91.18% <100.00%> (+0.44%) ⬆️
hathor/transaction/transaction_metadata.py 90.90% <100.00%> (+1.17%) ⬆️

... and 11 files with indirect coverage changes

@jansegre jansegre force-pushed the feat/validation-safeguards branch from 13c1243 to e4ab48e Compare May 23, 2023 15:26
@jansegre jansegre force-pushed the feat/validation-safeguards branch from 31c3534 to 5005cb6 Compare May 24, 2023 15:42
@jansegre jansegre merged commit d80def3 into master May 24, 2023
@jansegre jansegre deleted the feat/validation-safeguards branch May 24, 2023 16:04
@jansegre jansegre mentioned this pull request Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants