Skip to content
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

Verify transactions in a block are well formed #4490

Merged
merged 1 commit into from
Oct 27, 2023

Conversation

yacovm
Copy link
Contributor

@yacovm yacovm commented Oct 25, 2023

Verify that transactions in blocks appear exactly as their marshaled form after unmarshaling.

@yacovm yacovm requested a review from a team as a code owner October 25, 2023 19:44
protoutil/blockutils.go Outdated Show resolved Hide resolved
@denyeart
Copy link
Contributor

At least a small PR description would be nice.

@yacovm
Copy link
Contributor Author

yacovm commented Oct 25, 2023

At least a small PR description would be nice.

Added a description

@yacovm yacovm force-pushed the wellForm branch 7 times, most recently from 3f4c8f2 to bfa28a2 Compare October 26, 2023 11:51
protoutil/blockutils.go Outdated Show resolved Hide resolved
protoutil/blockutils.go Outdated Show resolved Hide resolved
Verify that transactions in blocks appear exactly as their marshaled form after unmarshaling.

Signed-off-by: Yacov Manevich <yacov.manevich@ibm.com>
Copy link
Contributor

@ale-linux ale-linux left a comment

Choose a reason for hiding this comment

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

LGTM overall. The only thing that isn't clear yet is how we can convince ourselves that we're calling VerifyTransactionsAreWellFormed in all the right places to achieve complete mediation. I see that we call it in 3 places - how can we be sure these are the only three places?

protoutil/blockutils.go Show resolved Hide resolved
}
if !bytes.Equal(expected, rawTx) {
return fmt.Errorf("transaction %d (%s) does not match its raw form (%s)", i,
base64.StdEncoding.EncodeToString(expected), base64.StdEncoding.EncodeToString(rawTx))
Copy link
Contributor

Choose a reason for hiding this comment

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

why not hex encode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's less characters this way

@yacovm yacovm merged commit 45ebe47 into hyperledger:main Oct 27, 2023
14 checks passed
@yacovm
Copy link
Contributor Author

yacovm commented Oct 27, 2023

LGTM overall. The only thing that isn't clear yet is how we can convince ourselves that we're calling VerifyTransactionsAreWellFormed in all the right places to achieve complete mediation. I see that we call it in 3 places - how can we be sure these are the only three places?

Not only that we can't, but there is a stack of PRs pending to be reviewed that will need to be re-evaluated that we call it in different places.

The right approach would be to change the function that computes the hash of the block, to receive as parameter the entire block, not just the header.

@tock-ibm
Copy link
Contributor

LGTM overall. The only thing that isn't clear yet is how we can convince ourselves that we're calling VerifyTransactionsAreWellFormed in all the right places to achieve complete mediation. I see that we call it in 3 places - how can we be sure these are the only three places?

Not only that we can't, but there is a stack of PRs pending to be reviewed that will need to be re-evaluated that we call it in different places.

The right approach would be to change the function that computes the hash of the block, to receive as parameter the entire block, not just the header.

I'll take this commit and incorporate it into my stack of PRs, here:
#4346
#4408

@yacovm
Copy link
Contributor Author

yacovm commented Oct 27, 2023

@denyeart we can backport it now

denyeart added a commit to denyeart/fabric that referenced this pull request Oct 27, 2023
Verify that transactions in blocks appear exactly as their marshaled form after unmarshaling.

Backport of hyperledger#4490

Signed-off-by: David Enyeart <enyeart@us.ibm.com>
denyeart added a commit to denyeart/fabric that referenced this pull request Oct 27, 2023
Verify that transactions in blocks appear exactly as their marshaled form after unmarshaling.

Backport of hyperledger#4490

Signed-off-by: David Enyeart <enyeart@us.ibm.com>
denyeart added a commit to denyeart/fabric that referenced this pull request Oct 27, 2023
Verify that transactions in blocks appear exactly as their marshaled form after unmarshaling.

Backport of hyperledger#4490

Signed-off-by: David Enyeart <enyeart@us.ibm.com>
denyeart added a commit to denyeart/fabric that referenced this pull request Oct 27, 2023
Verify that transactions in blocks appear exactly as their marshaled form after unmarshaling.

Backport of hyperledger#4490

Signed-off-by: David Enyeart <enyeart@us.ibm.com>
denyeart added a commit to denyeart/fabric that referenced this pull request Oct 27, 2023
Verify that transactions in blocks appear exactly as their marshaled form after unmarshaling.

Backport of hyperledger#4490

Signed-off-by: David Enyeart <enyeart@us.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants