Skip to content

abci: Refactor CheckTx to notify of recheck#3744

Merged
melekes merged 11 commits intomasterfrom
2127-abci-recheck
Jul 2, 2019
Merged

abci: Refactor CheckTx to notify of recheck#3744
melekes merged 11 commits intomasterfrom
2127-abci-recheck

Conversation

@thanethomson
Copy link
Contributor

@thanethomson thanethomson commented Jun 21, 2019

As per #2127, this refactors the RequestCheckTx ProtoBuf struct to allow for a flag indicating whether a query is a recheck or not (and allows for possible future, more nuanced states).

In order to pass this extended information through to the ABCI app, the proxy.AppConnMempool (and, for consistency, the proxy.AppConnConsensus) interface seems to need to be refactored along with abcicli.Client.

And, as per this comment, I've made the following modification to the protobuf definition for the RequestCheckTx structure:

enum CheckTxType {
  New = 0;
  Recheck = 1;
}

message RequestCheckTx {
  bytes tx = 1;
  CheckTxType type = 2;
}
  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG_PENDING.md

As per #2127, this refactors the `RequestCheckTx` ProtoBuf struct to allow for:

1. a flag indicating whether a query is a recheck or not (and allows for
   possible future, more nuanced states)
2. an `additional_data` bytes array to provide information for those more
   nuanced states.

In order to pass this extended information through to the ABCI app, the
`proxy.AppConnMempool` (and, for consistency, the
`proxy.AppConnConsensus`) interface seems to need to be refactored.
@thanethomson thanethomson requested review from ebuchman and melekes June 21, 2019 15:26
@thanethomson thanethomson self-assigned this Jun 21, 2019
@codecov-io
Copy link

codecov-io commented Jun 21, 2019

Codecov Report

Merging #3744 into master will decrease coverage by 0.05%.
The diff coverage is 60%.

@@            Coverage Diff             @@
##           master    #3744      +/-   ##
==========================================
- Coverage   63.88%   63.82%   -0.06%     
==========================================
  Files         217      217              
  Lines       18071    18124      +53     
==========================================
+ Hits        11545    11568      +23     
- Misses       5553     5575      +22     
- Partials      973      981       +8
Impacted Files Coverage Δ
proxy/app_conn.go 0% <0%> (ø) ⬆️
state/execution.go 73.41% <100%> (ø) ⬆️
mempool/clist_mempool.go 81.72% <100%> (+0.18%) ⬆️
privval/signer_validator_endpoint.go 75.55% <0%> (-10%) ⬇️
privval/socket_listeners.go 86.2% <0%> (-3.45%) ⬇️
evidence/store.go 92.59% <0%> (+0.81%) ⬆️
blockchain/reactor.go 71.49% <0%> (+0.93%) ⬆️
libs/db/remotedb/remotedb.go 40.83% <0%> (+4.93%) ⬆️

@thanethomson thanethomson marked this pull request as ready for review June 22, 2019 14:49
@thanethomson thanethomson requested a review from xla as a code owner June 22, 2019 14:49
@tac0turtle tac0turtle changed the base branch from develop to master June 26, 2019 15:33
@tac0turtle tac0turtle added ready-for-review C:abci Component: Application Blockchain Interface labels Jul 1, 2019
Copy link
Contributor

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

🥇

Copy link
Contributor

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

This is great! Thanks for implementing it!

@melekes melekes merged commit 62f97a6 into master Jul 2, 2019
@melekes melekes deleted the 2127-abci-recheck branch July 2, 2019 14:14
Copy link
Contributor

@ebuchman ebuchman left a comment

Choose a reason for hiding this comment

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

Awesome

- **Request**:
- `Tx ([]byte)`: The request transaction bytes
- `Type (CheckTxType)`: What type of `CheckTx` request is this? At present,
there are two possible values: `CheckTx_Unchecked` (the default, which says
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, seems this is not aligned with the changes actually made to the proto file, ie. the types are New and Recheck

Copy link
Contributor

Choose a reason for hiding this comment

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

We should actually also add an entry to docs/spec/abci/apps.md explaining the impact of this field on application developers (ie. they may be able to skip signature verification)

there are two possible values: `CheckTx_Unchecked` (the default, which says
that a full check is required), and `CheckTx_Checked` (when the mempool is
initiating a normal recheck of a transaction).
- `AdditionalData ([]byte)`: Reserved for future use. See
Copy link
Contributor

Choose a reason for hiding this comment

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

This wasn't implemented yet

@ebuchman ebuchman mentioned this pull request Jul 12, 2019
5 tasks
tac0turtle pushed a commit that referenced this pull request Jul 12, 2019
* Update ABCI docs to reflect latest changes on PR #3744

* Add note about new Type parameter for CheckTx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C:abci Component: Application Blockchain Interface

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants