-
Notifications
You must be signed in to change notification settings - Fork 970
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
Make tx set frame order matter only during encoding/decoding. #3496
Conversation
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.
Thank you
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.
looks good, minor test issue. you can squash commits and we'll merge
@@ -2377,12 +2377,12 @@ TEST_CASE_VERSIONS("txenvelope", "[tx][envelope]") | |||
|
|||
auto r = closeLedgerOn(*app, 1, 2, 2016, {tx1, tx2}); | |||
|
|||
REQUIRE(tx1->getResultCode() == txSUCCESS); | |||
REQUIRE(tx2->getResultCode() == txFAILED); | |||
checkTx(0, r, txSUCCESS); |
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.
I think you need to pass true
to the last arg of closeLedgerOn
, otherwise you don't know if tx1 < tx2
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.
I don't think so: results are in the apply order (I've double-checked with the debugger). The last arg of closeLedgerOn
makes the txs to be applied in the same order as we have passed to the method (which already happens due to seq nums) and it has nothing to do with the result order.
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.
Ah didn't see that tx1 and tx2 were from the same account!
There is no reason to maintain the order in memory and then implicitly assume it's preserved during encoding. Instead, we store transactions in arbitrary order in memory and sort only during encoding to XDR. This hopefully should reduce the risk of outputting a malformed tx set. For additional safety, we do the XDR roundtrip during the in-memory tx set construction, so we guarantee that we never nominate malformed tx set XDR (at least from the standpoint of the particular core version).
604a55c
to
1f5d4bc
Compare
r+ 1f5d4bc |
Description
Resolves #3495
Make tx set frame order matter only during encoding/decoding.
There is no reason to maintain the order in memory and then implicitly assume it's preserved during encoding. Instead, we store transactions in arbitrary order in memory and sort only during encoding to XDR. This hopefully should reduce the risk of outputting a malformed tx set.
Checklist
clang-format
v8.0.0 (viamake format
or the Visual Studio extension)