-
Notifications
You must be signed in to change notification settings - Fork 366
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ST-541 Fix creation of audit ledger transactions #1154
Conversation
…multiple ledgers in one batch Signed-off-by: Nikita Khateev <nikita.khateev@dsr-corporation.com>
Signed-off-by: Nikita Khateev <nikita.khateev@dsr-corporation.com>
Can one of the admins verify this patch? |
Could one of the committers please verify this patch? |
last_audit_txn_data = get_payload_data(last_audit_txn) if last_audit_txn is not None else None | ||
|
||
# 1. ledger is changed in this batch => root_hash | ||
if lid == target_ledger_id: | ||
txn[AUDIT_TXN_LEDGER_ROOT][lid] = Ledger.hashToStr(three_pc_batch.txn_root) | ||
if ledger.uncommittedTxns: |
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.
We may have multiple uncommitted txns for the same ledger in fly, so that a check if ledger.uncommittedTxns
doesn't man that a ledger was changed in this batch.
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.
Fixed
Signed-off-by: Nikita Khateev <nikita.khateev@dsr-corporation.com>
@@ -98,7 +98,7 @@ def _create_audit_txn_data(self, three_pc_batch, last_audit_txn): | |||
|
|||
# 3. ledger root (either root_hash or seq_no to last changed) | |||
# TODO: support setting for multiple ledgers | |||
self.__fill_ledger_root_hash(txn, three_pc_batch, lid, last_audit_txn) | |||
self.__fill_ledger_root_hash(txn, lid, ledger, last_audit_txn) | |||
|
|||
# 4. state root hash | |||
txn[AUDIT_TXN_STATE_ROOT][three_pc_batch.ledger_id] = Ledger.hashToStr(three_pc_batch.state_root) |
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.
We need to update AUDIT_TXN_STATE_ROOT
for all changed ledgers as well.
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.
Fixed
|
||
# 2. This ledger is never audited, so do not add the key | ||
# 1.1. Rare case -- we have previous audit txns but don't have this ledger i.e. new plugins |
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.
Please add unit tests for each of this cases
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.
Enhanced old unit test and added a new one for the rare case
Signed-off-by: Nikita Khateev <nikita.khateev@dsr-corporation.com>
(ci) test this please |
Signed-off-by: Nikita Khateev <nikita.khateev@dsr-corporation.com>
(ci) test this please |
Signed-off-by: Nikita Khateev <nikita.khateev@dsr-corporation.com>
txn_data = audit_batch_handler._create_audit_txn_data(batch, audit_batch_handler.ledger.get_last_txn()) | ||
|
||
# Checking first batch created | ||
assert txn_data[AUDIT_TXN_LEDGER_ROOT][1] == domain_root_hash_2 |
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.
It would be great to use check_audit_txn
in these tests.
It now works for multiple ledgers in one batch