Skip to content
This repository has been archived by the owner on Feb 1, 2024. It is now read-only.

fix(commit_store): fix bug in get_batch_by_transaction #2458

Open
wants to merge 3 commits into
base: 1-3
Choose a base branch
from

Conversation

suchapalaver
Copy link
Contributor

@suchapalaver suchapalaver commented Sep 17, 2023

This PR fixes a bug in the sawtooth_validator::journal::commit_store::CommitStore's get_batch_by_transaction method, which heretofore returned the first batch that does not contain the provided transaction_id argument without any error.

The equivalent Python method calls the Rust method via FFI:

# validator/sawtooth_validator/journal/block_store.py

def get_batch_by_transaction(self, transaction_id):
        """
        Check to see if the requested transaction_id is in the current chain.
        If so, find the batch that has the transaction referenced by the
        transaction_id and return the batch. This is done by finding the block
        and searching for the batch.

        :param transaction_id (string): The id of the transaction that is being
            requested.
        :return:
        The batch that has the transaction.
        """
        payload = self._get_data_by_id(
            transaction_id, 'commit_store_get_batch_by_transaction')

        batch = Batch()
        batch.ParseFromString(payload)

        return batch

Here's commit_store_get_batch_by_transaction, which is called in the snippet above and calls get_batch_by_transaction:

//! validator/src/journal/commit_store_ffi.rs

#[no_mangle]
pub unsafe extern "C" fn commit_store_get_batch_by_transaction(
    commit_store: *mut c_void,
    transaction_id: *const c_char,
    batch_ptr: *mut *const u8,
    batch_len: *mut usize,
    batch_cap: *mut usize,
) -> ErrorCode {
    check_null!(commit_store, transaction_id);

    match deref_cstr(transaction_id) {
        Ok(transaction_id) => {
            match (*(commit_store as *mut CommitStore)).get_batch_by_transaction(transaction_id) {
                Ok(batch) => return_batch(batch, batch_ptr, batch_len, batch_cap),
                Err(err) => map_database_error(err),
            }
        }
        Err(err) => err,
    }
}

Concomitant changes were merged into sawtooth-lib in this PR.

Original work

Signed-off-by: Joseph Livesey [email protected]

@suchapalaver suchapalaver force-pushed the fix/fix-bug-in-get-batch-by-transaction branch from 7e44a29 to d615928 Compare September 17, 2023 14:59
@suchapalaver suchapalaver changed the title fix 'derivable_impl' clippy warning fix(commit_store): fix bug in get_batch_by_transaction Sep 17, 2023
@suchapalaver suchapalaver force-pushed the fix/fix-bug-in-get-batch-by-transaction branch from d615928 to 08b26b7 Compare September 17, 2023 15:03
@suchapalaver suchapalaver marked this pull request as ready for review September 17, 2023 15:03
@suchapalaver suchapalaver force-pushed the fix/fix-bug-in-get-batch-by-transaction branch 4 times, most recently from 3a1f377 to 6b5ac34 Compare September 22, 2023 15:42
@suchapalaver suchapalaver force-pushed the fix/fix-bug-in-get-batch-by-transaction branch from 6b5ac34 to 4298afa Compare October 1, 2023 19:30
@suchapalaver suchapalaver force-pushed the fix/fix-bug-in-get-batch-by-transaction branch 2 times, most recently from b9739d9 to b18178f Compare October 9, 2023 22:20
@suchapalaver suchapalaver marked this pull request as draft October 9, 2023 22:28
@@ -322,7 +322,7 @@ impl CommitStore {
!batch
.transaction_ids
.iter()
.any(|txn_id| txn_id == transaction_id)
.all(|txn_id| txn_id != transaction_id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it also work to just remove the "!" before "batch"? (If so, it would keep the performance of using any instead of all.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vaporos done.

@suchapalaver suchapalaver force-pushed the fix/fix-bug-in-get-batch-by-transaction branch 2 times, most recently from 57495e5 to 47a8975 Compare October 10, 2023 15:08
@suchapalaver suchapalaver marked this pull request as ready for review October 10, 2023 15:09
@suchapalaver suchapalaver force-pushed the fix/fix-bug-in-get-batch-by-transaction branch 4 times, most recently from 2462780 to 75f7f06 Compare October 10, 2023 15:52
@suchapalaver suchapalaver marked this pull request as draft October 10, 2023 17:18
@vaporos
Copy link
Contributor

vaporos commented Nov 20, 2023

Rebase should resolve the jenkins build error (it should be excluded).

@suchapalaver suchapalaver force-pushed the fix/fix-bug-in-get-batch-by-transaction branch from 031669a to 7e3088c Compare November 20, 2023 22:07
@suchapalaver suchapalaver requested a review from vaporos November 20, 2023 22:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants