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

abci: use EndBlock to control empty block creation #1909

Open
ebuchman opened this issue Jul 5, 2018 · 23 comments
Open

abci: use EndBlock to control empty block creation #1909

ebuchman opened this issue Jul 5, 2018 · 23 comments

Comments

@ebuchman
Copy link
Contributor

@ebuchman ebuchman commented Jul 5, 2018

Since some apps will change the app hash every block, even with empty blocks, setting create_empty_blocks=false won't achieve anything.

To solve this, we could add a Pause bool field to ResponseEndBlock that tells Tendermint whether to make a new block right away or not.

We should write a proper ADR before making this change. Note that any state transitions that occur in the last block committed will not have light-client proofs until the next block is committed, so apps need to be careful when setting Pause=true

@b00f
Copy link
Contributor

@b00f b00f commented Jul 11, 2018

It's good idea giving the application this privilege to decide when pausing block creation.

Loading

@ebuchman
Copy link
Contributor Author

@ebuchman ebuchman commented Oct 29, 2018

Another thing we could consider:

We could remove the dependence on AppHash in the meantime, and only make blocks if there are transactions (or evidence). It means we won't have proofs for time-dependent transitions in the empty proofBlock, but perhaps that is worth it to eliminate the confusion in this feature. We'd still need to make an empty proofBlock if the last block has transactions so we can get proofs for the new state resulting from the previous block.

Loading

@jdkanani
Copy link

@jdkanani jdkanani commented Nov 1, 2018

@ebuchman Can we move "AppHash change" in EndBlock or StateCommit (introduce new one) hook and assign AppHash in current block before commit (like validator set)? I am not sure any implications it might have.

Or atleast have a configuration flag to disable support for block proof?

Loading

@b00f
Copy link
Contributor

@b00f b00f commented Nov 1, 2018

how about processing but not commiting empty block? I mean every node agree that empty block won't change the state... Just an idea...

Loading

@KrzysiekJ
Copy link
Contributor

@KrzysiekJ KrzysiekJ commented Nov 10, 2018

Note that an application may want to punish absent validators and therefore application state may potentially change in every block. Also applications may want to use block timestamps to perform some time-dependent transitions, which also implies changing hash with every block. Therefore it may be hard to imagine a non-enterprise (local) application which doesn’t potentially change hash with every block, however light clients don’t necessarily need to be interested in such changes. Not depending on AppHash may be a reasonable default then.

Still there may be situations where an application wants to accelerate block creation or create next block in a predictable time, for example when it announces a change in the validator set, so either introducing a Pause boolean or at least creating a block if there is a pending change in the validator set may be a good idea.

Loading

@b00f
Copy link
Contributor

@b00f b00f commented Nov 11, 2018

So I think we agree that application can decide to commit an empty block or not. Imagine there is an empty block and application decide to not commit it. Then the previous round will be repeated with same validator set, note that most nodes are in consensus about it. In this case the empty block will be processed but not committed and app hash won't change (either the block height).

Loading

@ebuchman
Copy link
Contributor Author

@ebuchman ebuchman commented Nov 11, 2018

Someone want to write ADR-036 for this? It should include a mechanism for the app to control the rate of empty block production through ABCI, but I don't think we want it in the ConsensusParams, because we don't need it to be hashed, and it could conceivably stay a subjective/local/non-consensus setting.

Should we use Query for this - to ask the app if or when to make the next empty block? Maybe a path like /consensus/blocks/create_empty and /consensus/blocks/create_empty_interval ?

Loading

@jdkanani
Copy link

@jdkanani jdkanani commented Dec 6, 2018

@ebuchman may be a stupid question but what is ADR-036? Would love to fix whole empty block thing between tendermint/cosmos-sdk.

Loading

@ebuchman
Copy link
Contributor Author

@ebuchman ebuchman commented Dec 15, 2018

ADR-36 is an unwritten Architecture Decision Record justifying this change and describing details of its design. See other ADRs for examples: https://github.com/tendermint/tendermint/tree/develop/docs/architecture

Issue #2313 tracks the assignment of numbers to new ADRs.

Would be great if you want to start by writing this up.

Loading

@jdkanani
Copy link

@jdkanani jdkanani commented Dec 17, 2018

@ebuchman Great. I will work on it.

Loading

@b00f
Copy link
Contributor

@b00f b00f commented Feb 28, 2019

I would like to share my experience since it may help you addressing this issue.
If there is no transaction in the block (empty block), application hash SHOULD NOT change. In case application hash changes by committing an empty block, it is due to a BUG or naive implementation of the application. I think Tendermint should not be responsible in this case and application should be cautious to not change its state when nothing is happening.

Also look here: cosmos/iavl#128

Loading

@ebuchman
Copy link
Contributor Author

@ebuchman ebuchman commented Feb 28, 2019

Yeh that's a nice idea but I'm not sure it's feasible in practice - apps may want to do things like store the header in the state, or otherwise change the state based on timers even if there's no txs. So we can't really assume that an empty block will have no effect on state

Loading

@ebuchman ebuchman removed this from the post-launch milestone Feb 28, 2019
@ebuchman ebuchman added this to the v0.33.0 milestone Feb 28, 2019
@b00f
Copy link
Contributor

@b00f b00f commented Mar 1, 2019

apps may want to do things like store the header in the state, or otherwise change the state based on timers even if there's no txs

Without reaching any consensus how can they store things like timestamp?

Did you check the iavl project. I found that this project always change that hash anytime the save method calls. I assume some applications using iavl for storing their data so I believe this code is devil here!

==UPDATE==
if there is any changes in App (state), definitely a block should be created. Example: Application might be interested to incentivize validator (or proposer) in each round by giving some coins. In this case the new block shall be created, since the App state has changed and therefor app hash has changed.

Loading

@KrzysiekJ
Copy link
Contributor

@KrzysiekJ KrzysiekJ commented Mar 1, 2019

@b00f Three use cases (from Ercoin):

  • Accounts have time-dependent validity; in each block it may happen that some accounts are expired (removed from the state).
  • Locked accounts are unlocked after specific time.
  • Absencies of validators are stored in the application state (and this information is used to grant rewards from transaction fees, though this does not happen with every block).

What benefit does creating an empty block give in these cases?

Loading

@b00f
Copy link
Contributor

@b00f b00f commented Mar 2, 2019

To better understand this special cases, If B stands for the block, S for the application state and n for the height, we will have these sequences in case an empty block is created:
{B[n], S[n]} => {B[n+1], S[n+1]}
However this sequence when the empty block (B[n+1]) won't be created will be:
{B[n], S[n,n+1,...]}
In this cases Application state changes but block won't. A little strange for me. One block can have several states. Don't you agree with me that the application state (hash) is part of the block and by changing it the block will be changed?
There will be two definitions here: empty blocks (no txs), empty and identical blocks(no txs and same app hash)

Loading

@KrzysiekJ
Copy link
Contributor

@KrzysiekJ KrzysiekJ commented Mar 3, 2019

In normal scenario each block determines only one state. The hash of this state is signed by the validator set in the next block, but can be calculated without these signatures (without next block). Empty block may be needed by light clients which are not capable of calculating application state themselves.

Loading

@b00f
Copy link
Contributor

@b00f b00f commented Mar 4, 2019

but can be calculated without these signatures (without next block)

Data should be strictly deterministic (not something like timestamp or random data). My point is here: Tendermint should let application decides to make a block or not. If developers are confident enough the block can be ignored.

Loading

@ebuchman ebuchman removed this from the ABCI Updates milestone May 10, 2019
@ebuchman ebuchman added this to the v0.33.0 milestone May 10, 2019
@mdyring
Copy link
Contributor

@mdyring mdyring commented Sep 13, 2019

I'd suggest "needProofBlock" as a better name than pause.

Is there any plans to implement this in the near future? It seems more and more chains are based on cosmos-sdk and I imagine the added flexibility with regards to block generation would be a welcomed addition.

Loading

@marbar3778
Copy link
Member

@marbar3778 marbar3778 commented Oct 10, 2019

This is currently in the milestone for 0.33, we would like to complete it in the near future. @mdyring would you like to write up ADR-036 to get the ball rolling?

Loading

@marbar3778 marbar3778 removed this from the v0.33.0 milestone Nov 12, 2019
silasdavis added a commit to hyperledger/burrow that referenced this issue Apr 18, 2020
StreamEvents.

When serialising BlockExecutions into StreamEvents it is possible that
we do not detect dropped SteamEvents when deserialising (consuming) the SteamEvents later.

This PR makes the StreamEvent consumers, namely: ConsumeBlockExecutions, BlockAccumulator, and TxStack throw errors if there is an incorrect number of events streamed within each container (BlockExecution or TxExecution). To do this a checksum NumTxs and NumEvents is added to BeginBlock and BeginTx respectively.

This allows Vent to crash if it sees an invalid stream (for example due
to messages being dropped due to load on Burrow's event emitter) or a
transport issue. Vent can then restart from its previous good offset.

Annoyance: I had to add in Predecessor to the BlockExecution and
BeginBlock messages in order to track the previous _expected_ block
since we do not store empty blocks in state to avoid changing the AppHash (which itself is just a
workaround for an outstanding issue in tendermint
tendermint/tendermint#1909)

This PR was in response to suspected event drops in Vent in production.

Signed-off-by: Silas Davis <silas@monax.io>
silasdavis added a commit to hyperledger/burrow that referenced this issue Apr 19, 2020
StreamEvents.

When serialising BlockExecutions into StreamEvents it is possible that
we do not detect dropped SteamEvents when deserialising (consuming) the SteamEvents later.

This PR makes the StreamEvent consumers, namely: ConsumeBlockExecutions, BlockAccumulator, and TxStack throw errors if there is an incorrect number of events streamed within each container (BlockExecution or TxExecution). To do this a checksum NumTxs and NumEvents is added to BeginBlock and BeginTx respectively.

This allows Vent to crash if it sees an invalid stream (for example due
to messages being dropped due to load on Burrow's event emitter) or a
transport issue. Vent can then restart from its previous good offset.

Annoyance: I had to add in Predecessor to the BlockExecution and
BeginBlock messages in order to track the previous _expected_ block
since we do not store empty blocks in state to avoid changing the AppHash (which itself is just a
workaround for an outstanding issue in tendermint
tendermint/tendermint#1909)

This PR was in response to suspected event drops in Vent in production.

Signed-off-by: Silas Davis <silas@monax.io>
silasdavis added a commit to hyperledger/burrow that referenced this issue Apr 19, 2020
StreamEvents.

When serialising BlockExecutions into StreamEvents it is possible that
we do not detect dropped SteamEvents when deserialising (consuming) the SteamEvents later.

This PR makes the StreamEvent consumers, namely: ConsumeBlockExecutions, BlockAccumulator, and TxStack throw errors if there is an incorrect number of events streamed within each container (BlockExecution or TxExecution). To do this a checksum NumTxs and NumEvents is added to BeginBlock and BeginTx respectively.

This allows Vent to crash if it sees an invalid stream (for example due
to messages being dropped due to load on Burrow's event emitter) or a
transport issue. Vent can then restart from its previous good offset.

Annoyance: I had to add in Predecessor to the BlockExecution and
BeginBlock messages in order to track the previous _expected_ block
since we do not store empty blocks in state to avoid changing the AppHash (which itself is just a
workaround for an outstanding issue in tendermint
tendermint/tendermint#1909)

This PR was in response to suspected event drops in Vent in production.

Signed-off-by: Silas Davis <silas@monax.io>
silasdavis added a commit to hyperledger/burrow that referenced this issue Apr 19, 2020
StreamEvents.

When serialising BlockExecutions into StreamEvents it is possible that
we do not detect dropped SteamEvents when deserialising (consuming) the SteamEvents later.

This PR makes the StreamEvent consumers, namely: ConsumeBlockExecutions, BlockAccumulator, and TxStack throw errors if there is an incorrect number of events streamed within each container (BlockExecution or TxExecution). To do this a checksum NumTxs and NumEvents is added to BeginBlock and BeginTx respectively.

This allows Vent to crash if it sees an invalid stream (for example due
to messages being dropped due to load on Burrow's event emitter) or a
transport issue. Vent can then restart from its previous good offset.

Annoyance: I had to add in Predecessor to the BlockExecution and
BeginBlock messages in order to track the previous _expected_ block
since we do not store empty blocks in state to avoid changing the AppHash (which itself is just a
workaround for an outstanding issue in tendermint
tendermint/tendermint#1909)

This PR was in response to suspected event drops in Vent in production.

Signed-off-by: Silas Davis <silas@monax.io>
silasdavis added a commit to hyperledger/burrow that referenced this issue Apr 20, 2020
StreamEvents.

When serialising BlockExecutions into StreamEvents it is possible that
we do not detect dropped SteamEvents when deserialising (consuming) the SteamEvents later.

This PR makes the StreamEvent consumers, namely: ConsumeBlockExecutions, BlockAccumulator, and TxStack throw errors if there is an incorrect number of events streamed within each container (BlockExecution or TxExecution). To do this a checksum NumTxs and NumEvents is added to BeginBlock and BeginTx respectively.

This allows Vent to crash if it sees an invalid stream (for example due
to messages being dropped due to load on Burrow's event emitter) or a
transport issue. Vent can then restart from its previous good offset.

Annoyance: I had to add in Predecessor to the BlockExecution and
BeginBlock messages in order to track the previous _expected_ block
since we do not store empty blocks in state to avoid changing the AppHash (which itself is just a
workaround for an outstanding issue in tendermint
tendermint/tendermint#1909)

This PR was in response to suspected event drops in Vent in production.

Signed-off-by: Silas Davis <silas@monax.io>
silasdavis added a commit to hyperledger/burrow that referenced this issue Apr 20, 2020
StreamEvents.

When serialising BlockExecutions into StreamEvents it is possible that
we do not detect dropped SteamEvents when deserialising (consuming) the SteamEvents later.

This PR makes the StreamEvent consumers, namely: ConsumeBlockExecutions, BlockAccumulator, and TxStack throw errors if there is an incorrect number of events streamed within each container (BlockExecution or TxExecution). To do this a checksum NumTxs and NumEvents is added to BeginBlock and BeginTx respectively.

This allows Vent to crash if it sees an invalid stream (for example due
to messages being dropped due to load on Burrow's event emitter) or a
transport issue. Vent can then restart from its previous good offset.

Annoyance: I had to add in Predecessor to the BlockExecution and
BeginBlock messages in order to track the previous _expected_ block
since we do not store empty blocks in state to avoid changing the AppHash (which itself is just a
workaround for an outstanding issue in tendermint
tendermint/tendermint#1909)

This PR was in response to suspected event drops in Vent in production.

Signed-off-by: Silas Davis <silas@monax.io>
silasdavis added a commit to hyperledger/burrow that referenced this issue Apr 21, 2020
StreamEvents.

When serialising BlockExecutions into StreamEvents it is possible that
we do not detect dropped SteamEvents when deserialising (consuming) the SteamEvents later.

This PR makes the StreamEvent consumers, namely: ConsumeBlockExecutions, BlockAccumulator, and TxStack throw errors if there is an incorrect number of events streamed within each container (BlockExecution or TxExecution). To do this a checksum NumTxs and NumEvents is added to BeginBlock and BeginTx respectively.

This allows Vent to crash if it sees an invalid stream (for example due
to messages being dropped due to load on Burrow's event emitter) or a
transport issue. Vent can then restart from its previous good offset.

Annoyance: I had to add in Predecessor to the BlockExecution and
BeginBlock messages in order to track the previous _expected_ block
since we do not store empty blocks in state to avoid changing the AppHash (which itself is just a
workaround for an outstanding issue in tendermint
tendermint/tendermint#1909)

This PR was in response to suspected event drops in Vent in production.

Signed-off-by: Silas Davis <silas@monax.io>
silasdavis added a commit to hyperledger/burrow that referenced this issue Apr 21, 2020
StreamEvents.

When serialising BlockExecutions into StreamEvents it is possible that
we do not detect dropped SteamEvents when deserialising (consuming) the SteamEvents later.

This PR makes the StreamEvent consumers, namely: ConsumeBlockExecutions, BlockAccumulator, and TxStack throw errors if there is an incorrect number of events streamed within each container (BlockExecution or TxExecution). To do this a checksum NumTxs and NumEvents is added to BeginBlock and BeginTx respectively.

This allows Vent to crash if it sees an invalid stream (for example due
to messages being dropped due to load on Burrow's event emitter) or a
transport issue. Vent can then restart from its previous good offset.

Annoyance: I had to add in Predecessor to the BlockExecution and
BeginBlock messages in order to track the previous _expected_ block
since we do not store empty blocks in state to avoid changing the AppHash (which itself is just a
workaround for an outstanding issue in tendermint
tendermint/tendermint#1909)

This PR was in response to suspected event drops in Vent in production.

Signed-off-by: Silas Davis <silas@monax.io>
@tessr
Copy link
Member

@tessr tessr commented Apr 24, 2020

I'm removing this from milestone 0.34 (tentatively scheduled for mid-to-late May), since it still needs an ADR and isn't a priority for the other features in that milestone. I'll keep it in milestone 1.0 so it doesn't get deprioritized completely. If anyone feels differently, please speak up.

Loading

@tessr tessr removed this from the v0.34 Block Protocol Breaking Release milestone Apr 24, 2020
@tessr tessr added this to the v1.0 milestone Apr 24, 2020
@tessr tessr added this to Untriaged 🥚 in Tendermint Core Project Board via automation Nov 16, 2020
@tessr tessr moved this from Untriaged 🥚 to Icebox 🧊 in Tendermint Core Project Board Nov 16, 2020
@marbar3778
Copy link
Member

@marbar3778 marbar3778 commented Nov 8, 2021

I'd suggest "needProofBlock" as a better name than pause.

Is there any plans to implement this in the near future? It seems more and more chains are based on cosmos-sdk and I imagine the added flexibility with regards to block generation would be a welcomed addition.

@mdyring this issue has come up recently again. We wanted to see if you would still like to use it

Loading

@mdyring
Copy link
Contributor

@mdyring mdyring commented Nov 8, 2021

@marbar3778 yeah I think this would be a nice addition, what do you think @haasted and @blewater?

Loading

@haasted
Copy link
Contributor

@haasted haasted commented Nov 9, 2021

I think it would still be a valuable addition. I believe that the create_empty_blocks=false setting is currently largely useless when some of the standard SDK modules are enabled, notably staking.

Our solution to this has been to move stuff out of the app_state and into an external database. Being able to just signal that a finalizing block is not needed for this state change seems to be the superior option to me.

Loading

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

Successfully merging a pull request may close this issue.

None yet
8 participants