Batch Amendment #5060
Batch Amendment #5060
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5060 +/- ##
=========================================
+ Coverage 77.9% 78.0% +0.1%
=========================================
Files 782 785 +3
Lines 66614 66970 +356
Branches 8159 8161 +2
=========================================
+ Hits 51861 52206 +345
- Misses 14753 14764 +11
... and 11 files with indirect coverage changes |
This is not finished. Needs to only revert the transactions submitted not the entire open ledger.
|
Also, since it seems this PR is still under active development, could you convert it to a draft? |
- allow only one flag - validate batch signers array (max and valid) - move `calculateBaseFee` - move `preflight2` - no duplicate TxIDs - no duplicate BatchSigners
|
|
||
| if (flags & tfUntilFailure) | ||
| { | ||
| result = tesSUCCESS; |
There was a problem hiding this comment.
so it's possible that none of the inner txns succeeds and yet the batch txn is "successful"?
There was a problem hiding this comment.
With this flag, the first failure would trigger an exit. Are you thinking we exit with tecBATCH_FAILURE? I think I agree
| void | ||
| ApplyContext::applyOpenView(OpenView& open) | ||
| { | ||
| if (!base_.open()) |
There was a problem hiding this comment.
could you elaborate more on why we are applying when the base is closed, and not when open? perhaps it's helpful to put more into the comment as well. thanks!
| if (pkSigner.empty()) | ||
| { | ||
| STArray const& txSigners(signer.getFieldArray(sfSigners)); | ||
| ret = checkMultiSign(ctx.view, idAccount, txSigners, ctx.j); |
There was a problem hiding this comment.
| ret = checkMultiSign(ctx.view, idAccount, txSigners, ctx.j); | |
| if (ret = checkMultiSign(ctx.view, idAccount, txSigners, ctx.j); | |
| !isTesSuccess(ret)) | |
| return ret; |
currently, it would return code of the result of the last signature, but shouldn't we be returning the first error that occurs in the loop? If so, we should probably add a test case to exercise the behavior where the last signer signature is correct, but previous ones are bad.
There was a problem hiding this comment.
Thanks for the suggestion. I will add a test case as well
| if (!sleAccount) | ||
| return tesSUCCESS; | ||
|
|
||
| ret = checkSingleSign( |
| ApplyViewImpl& avi = dynamic_cast<ApplyViewImpl&>(ctx_.view()); | ||
| std::vector<STObject> executions; | ||
| avi.copyBatchMetaData(executions); | ||
| ctx_.discard(); | ||
| ApplyViewImpl& avi2 = dynamic_cast<ApplyViewImpl&>(ctx_.view()); | ||
| avi2.setBatchMetaData(std::move(executions)); |
There was a problem hiding this comment.
what is this block of code here for?
There was a problem hiding this comment.
This is to apply the batch executions to the metadata even in the event of a tec failure
There was a problem hiding this comment.
we should add a comment above this code block as what you described.
also should we consider adding a if-condition where this block is executed conditionally only if the transaction type is Batch?
| if (not3rdParty) | ||
| ctx_.batchPrevious(avi); | ||
|
|
||
| ctx_.applyOpenView(subView); |
There was a problem hiding this comment.
Also just curious, does using subView.apply(ctx_.rawView()) not solve this problem? assuming that subView contains all the changes for the inner txns, shouldn't it include the latest account root state of the "non3rdParty" account? Please correct me if I'm wrong.
| // Only update the account root entry if the batch transaction was | ||
| // not a 3rd party transaction | ||
| if (not3rdParty) | ||
| ctx_.updateAccountRootEntry(); |
There was a problem hiding this comment.
linking to this comment thread
I'm still unclear why exactly we need to explicitly update account root entry here. Does it not work if we apply the state changes from subView inside the Batch transactor?
There was a problem hiding this comment.
I get an error when I attempt to apply the subView to the open view. Maybe I'm doing something wrong?
There was a problem hiding this comment.
Yeah I don't believe the existing functions are sufficient. We may need to implement new apply functions to support nested views/sandbox if we decide to go this route
- Update Comments - Update/Change function names - Update/Change argument names - Remove unused preclaim - Fix exit out of signature check (multi & single)
| @@ -147,7 +159,8 @@ ApplyStateTable::apply( | |||
| } | |||
| auto const origNode = to.read(keylet::unchecked(item.first)); | |||
| auto curNode = item.second.second; | |||
| if ((type == &sfModifiedNode) && (*curNode == *origNode)) | |||
| if ((type == &sfModifiedNode) && (*curNode == *origNode) && | |||
| !isBatch) | |||
There was a problem hiding this comment.
if i'm understanding correctly, when the transaction type is Batch, we are going to add the previous and final fields of the object even though they are the same.
This doesn't sound too correct, please correct me if i'm wrong. TY
There was a problem hiding this comment.
We are going to add the batchPrevAcctRootFields. The curNode and origNode are the same on the batch txn because of the way its reapplied in updateAccountRootEntry. I guess handling this as its own case might be preferred for better understanding?
There was a problem hiding this comment.
we can just add a comment here to explain the situation. Also it might be worth add an assertion to make sure that nodes are not modified if it is a batch transaction - this might before useful for future debugging
// If it is a batch txn, the curNode and origNode are the same on the batch txn
// because of the way its reapplied in updateAccountRootEntry
bool const nodesUnmodified = *curNode == *origNode;
if ((type == &sfModifiedNode) && nodesUnmodified &&
!isBatch)
continue;
assert(isBatch && !nodesUnmodified)
| @@ -147,7 +159,8 @@ ApplyStateTable::apply( | |||
| } | |||
| auto const origNode = to.read(keylet::unchecked(item.first)); | |||
| auto curNode = item.second.second; | |||
| if ((type == &sfModifiedNode) && (*curNode == *origNode)) | |||
| if ((type == &sfModifiedNode) && (*curNode == *origNode) && | |||
| !isBatch) | |||
| continue; | |||
| std::uint16_t nodeType = curNode | |||
There was a problem hiding this comment.
can a batch transaction ever modify an object other than AccountRoot? if not, would be use good to assert that here.
There was a problem hiding this comment.
No I dont believe so. I will add that here.
| @@ -203,6 +216,13 @@ ApplyStateTable::apply( | |||
| prevs.emplace_back(obj); | |||
| } | |||
|
|
|||
| if (isBatch && nodeType == ltACCOUNT_ROOT && batchPrev) | |||
| { | |||
| // TODO: This could fail if the fields already exist | |||
There was a problem hiding this comment.
is this TODO done? also would love to learn more about this, as the comment itself isn't too comprehensive 😃
- Remove BatchTxn Object - Use Global Flag `tfInnerBatchTxn ` for inner batch txns - Remove Sequence & Ticket workaround - replace sfBatchTxn submit/relay validation with tfInnerBatchTxn - Remove Sequence 0 Requirement
| Serializer dataStart = startMultiSigningData(*this); | ||
| finishMultiSigningData(accountID, dataStart); | ||
| return dataStart.getData(); | ||
| }); |
There was a problem hiding this comment.
what's the rationale for not returning a Slice directly in this lambda?
| constexpr std::uint32_t tfUniversalV2 = tfFullyCanonicalSig | tfInnerBatchTxn; | ||
| constexpr std::uint32_t tfUniversalMask = ~(tfFullyCanonicalSig | tfInnerBatchTxn); |
There was a problem hiding this comment.
| constexpr std::uint32_t tfUniversalV2 = tfFullyCanonicalSig | tfInnerBatchTxn; | |
| constexpr std::uint32_t tfUniversalMask = ~(tfFullyCanonicalSig | tfInnerBatchTxn); | |
| constexpr std::uint32_t tfUniversal = tfFullyCanonicalSig | tfInnerBatchTxn; | |
| constexpr std::uint32_t tfUniversalMask = ~(tfFullyCanonicalSig | tfInnerBatchTxn); |
we can prob add tfInnerBatchTxn into tfUniversal directly, and check in Transactor::preflight1 if tfInnerBatchTxn is usable
| return tesSUCCESS; | ||
| } | ||
| JLOG(ctx.j.warn()) << "Batch: sfFee must be zero."; | ||
| return temBAD_FEE; |
There was a problem hiding this comment.
we should add a test that checks if inner transaction has a specified non zero transaction fee
| { | ||
| auto const seq = env.seq(alice); | ||
| auto const batchFee = feeDrops * 2; | ||
| env(batch::batch(alice, seq, batchFee, tfAllOrNothing), |
There was a problem hiding this comment.
should also add a test where batch transaction sets tfInnerBatchTxn
| batch::add(pay(alice, bob, XRP(10)), seq + 0), | ||
| batch::add(pay(bob, alice, XRP(5)), env.seq(bob)), | ||
| batch::sig(carol), | ||
| ter(temBAD_SIGNER)); |
There was a problem hiding this comment.
do we have a test where it alice's sig is also included in the BatchSigners? something like
env(batch::batch(alice, seq, batchFee, tfAllOrNothing),
batch::add(pay(alice, bob, XRP(10)), seq + 0),
batch::add(pay(bob, alice, XRP(5)), env.seq(bob)),
batch::sig(alice, bob),
ter(temBAD_SIGNER));
This should fail because alice's sig is already provided in the outer tx.
| env(batch::batch(alice, seq, batchFee, tfAllOrNothing), | ||
| batch::add(pay(alice, bob, XRP(10)), seq + 1), | ||
| batch::add(pay(bob, alice, XRP(5)), env.seq(bob)), | ||
| batch::sig(bob)); |
There was a problem hiding this comment.
could also possibly add a test where bob's signature is incorrect
There was a problem hiding this comment.
There was a problem hiding this comment.
i can't see which test the links points me to, i think it's a github bug. But what i mean when i say bad sig is when the TxnSignature has been tampered. do you think it's worth testing?
| env(batch::batch(alice, seq, batchFee, tfAllOrNothing), | ||
| batch::add(pay(alice, bob, XRP(1)), seq + 1), | ||
| batch::add(pay(alice, bob, XRP(1)), seq + 2), | ||
| msig(bob, carol)); |
There was a problem hiding this comment.
could have a test where either carol or bob's sig is bad
There was a problem hiding this comment.
This is just a multisigned txn. But I will add those two edge cases.
| env(batch::batch(alice, seq, batchFee, tfAllOrNothing), | ||
| batch::add(pay(alice, bob, XRP(10)), seq + 1), | ||
| batch::add(pay(bob, alice, XRP(5)), env.seq(bob)), | ||
| batch::msig(bob, {dave, carol})); |
There was a problem hiding this comment.
could also add a test where multi sign does not meet the quorum
There was a problem hiding this comment.
also might be worth having one or two more BatchSigner, this test only has one BatchSigner
| void | ||
| testNoAccount(FeatureBitset features) | ||
| { | ||
| testcase("no account"); |
There was a problem hiding this comment.
is this the right name for the test? seems like the test is just submitting a batch with payment and accountset. there is a purpose for testing this combo specifically?
There was a problem hiding this comment.
This test is submitting a payment to an account that does not exist and then doing the AccountSet. Its just there to test that account activation works. I can change the name? What name would you suggest? Or should I remove the test all together?
There was a problem hiding this comment.
I see that makes sense, might be worth aadding a comment like
// Create Bob's account by funding it with XRP as an inner transaction.
// Then Bob submits an AccountSet transaction as another inner transaction.
- fix comments - rename batch executions
| batch::add(pay(alice, bob, XRP(10)), preAliceSeq + 1), | ||
| batch::add(pay(bob, alice, XRP(5)), preBobSeq + 10), | ||
| batch::sig(bob), | ||
| ter(tecBATCH_FAILURE)); |
There was a problem hiding this comment.
should add another test to check where alice specifies the same sequence for both outer and inner txns:
env(batch::batch(alice, preAliceSeq, batchFee, tfAllOrNothing),
batch::add(pay(alice, bob, XRP(10)), preAliceSeq),
batch::add(pay(bob, alice, XRP(5)), preBobSeq),
batch::sig(bob),
ter(tecBATCH_FAILURE));
| ApplyViewImpl& avi = dynamic_cast<ApplyViewImpl&>(ctx_.view()); | ||
| std::vector<STObject> executions; | ||
| avi.copyBatchMetaData(executions); | ||
| ctx_.discard(); | ||
| ApplyViewImpl& avi2 = dynamic_cast<ApplyViewImpl&>(ctx_.view()); | ||
| avi2.setBatchMetaData(std::move(executions)); |
There was a problem hiding this comment.
we should add a comment above this code block as what you described.
also should we consider adding a if-condition where this block is executed conditionally only if the transaction type is Batch?
| { | ||
| JLOG(ctx.j.trace()) | ||
| << "Batch: Duplicate signer found: " << innerAccount; | ||
| return temINVALID_BATCH; |
There was a problem hiding this comment.
we need to add a unit test that has duplicated BatchSigner
| JLOG(ctx.j.trace()) | ||
| << "Batch: TransactionType missing in inner txn." | ||
| << "index: " << i; | ||
| return temINVALID_BATCH; |
There was a problem hiding this comment.
should add a test where an inner tx does not have TransactionType field
There was a problem hiding this comment.
Still have some concerns on the implementation detail:
- In Batch transactor, changes are only applied to the closed ledger and not open ledger, this will lead to wrong behaviors where the open ledger doesn't apply the changes from a Batch txn.
- I am still unsure if there is a need to store a var
batchPrevAcctRootFields_to explicitly manage previous account state. Would be nice if we are able to find a way without the need for the variable since it seems rather unclean.
| if (not3rdParty) | ||
| ctx_.batchPrevious(avi); | ||
|
|
||
| ctx_.applyOpenView(subView); |
There was a problem hiding this comment.
Makes me thinking, if we change the implementation of this function to simply do without the if-condition:
void
ApplyContext::applyOpenView(OpenView& open)
{
open.apply(base_);
}
This would unconditionally apply the states of the inner txn to the base_, which might avoid the need to have another batchPrevAcctRootFields_ to store the previous state of account root, and metadata generation might work by themselves.
Obviously I haven't tested my hypothesis and you might have already tried this approach (which might not work). But I would just like to have a bit more understanding on the decisions here (and why it doesn't work)
There was a problem hiding this comment.
we should also add a test where the outer account submits a few inner txn, causing its account state to change, and then we should validate the correctness of PreviousFields for the AccountRoot since we are introducing special logic for PreviousFields
High Level Overview of Change
Context of Change
Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)