Skip to content

abci++: update PrepareProposal API to accept just a list of bytes#9283

Merged
williambanfield merged 11 commits intofeature/abci++pppfrom
wb/option-3
Aug 18, 2022
Merged

abci++: update PrepareProposal API to accept just a list of bytes#9283
williambanfield merged 11 commits intofeature/abci++pppfrom
wb/option-3

Conversation

@williambanfield
Copy link
Contributor

@williambanfield williambanfield commented Aug 17, 2022

What does this change do?

This changes the ResponsePrepareProposal type, substituting the []TxRecord for just []bytes. This change is made in the .proto file and in all necessary additional places in the code.

The TxRecord type has been removed, as has the TxRecordSet along with the validation logic that the TxRecordSet implemented. This validation logic is no longer necessary. The tests for this type have been removed, as have the tests for ResponsePrepareProposal that tested that the TxRecord was used correctly.

Closes #9269

PR checklist

  • Tests written/updated, or no tests needed
  • CHANGELOG_PENDING.md updated, or no changelog entry needed
  • Updated relevant documentation (docs/) and code comments, or no
    documentation updates needed

@thanethomson thanethomson mentioned this pull request Aug 18, 2022
39 tasks
@williambanfield williambanfield changed the title draft of option 3 abci++: update PrepareProposal API to accept just a list of bytes Aug 18, 2022
@williambanfield williambanfield marked this pull request as ready for review August 18, 2022 17:43
@williambanfield williambanfield requested a review from a team August 18, 2022 17:43
@williambanfield
Copy link
Contributor Author

@sergio-mena should we open another issue to update the spec? I'm happy to take a stab at doing it here.

@sergio-mena
Copy link
Contributor

@williambanfield, since @jmalicevic is working on the v0.37.0 version of the spec, I think the best course of action is to add a checkbox here #9201, and let Jasmina know about it. Otherwise we risk stepping on each other's toes.

}

func (app *Application) CheckTx(req types.RequestCheckTx) types.ResponseCheckTx {
if req.Type == types.CheckTxType_Recheck {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure we want this if?
It would also make sense if we got a replay attempt for a transaction, wouldn't it?

Copy link
Contributor Author

@williambanfield williambanfield Aug 18, 2022

Choose a reason for hiding this comment

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

We clear the list of transactions to remove after every height, so this really only pertains to the recheck attempts that occur directly after the height. Replay attempts may happen but at the moment, the application does not guard against those at all otherwise, so I don't think we need to add that extra functionality to this code.

Copy link
Contributor

@sergio-mena sergio-mena left a comment

Choose a reason for hiding this comment

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

Only one minor comment on the example code.
Thanks a lot for the quick turnaround!

@williambanfield williambanfield added the S:automerge Automatically merge PR when requirements pass label Aug 18, 2022
@williambanfield williambanfield merged commit 1a4d939 into feature/abci++ppp Aug 18, 2022
@williambanfield williambanfield deleted the wb/option-3 branch August 18, 2022 20:33
@jmalicevic
Copy link
Contributor

Sorry to comment on this late, the spec says that there is a check for duplicate transactions in the transaction list returned via ResponsePrepareProposal. I could not find this in the code, is it there?

@sergio-mena
Copy link
Contributor

Excellent point, I'd say checking for duplicate transactions is something we'd still want to check at Tendermint level. Let's wait for @williambanfield to see what he thinks.
This shouldn't be gating our QA process, so not über-urgent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S:automerge Automatically merge PR when requirements pass

Projects

Status: Done/Merged

Development

Successfully merging this pull request may close these issues.

3 participants