types: Emit tags from BeginBlock/EndBlock #2747

kostko · 2018-11-02T09:30:37Z

This may need some discussion if the design seems sound. Note that this breaks the on-disk format due to including the BeginBlock field in the ABCIResponses structure.

Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGELOG_PENDING.md

melekes

🍓 🍌 🥛

Thanks for contributing to Tendermint! This is a good start. However, I am not confident about putting begin/end block results into EventNewBlock.

state/execution.go

kostko · 2018-11-05T10:03:19Z

I've addressed some design comments and rebased on the latest develop branch. PTAL.

kostko · 2018-11-05T10:12:49Z

It seems that test_apps test is failing due to an unrelated issue with Debian repositories and should be rerun.

melekes

Definitely a step in the right direction! A few comments though

state/execution.go

types/event_bus.go

types/event_bus_test.go

state/execution.go

kostko · 2018-11-09T08:52:51Z

Rebased on latest develop, updated docs and pending changelog. PTAL.

codecov-io · 2018-11-09T09:00:09Z

Codecov Report

Merging #2747 into develop will increase coverage by 0.01%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #2747      +/-   ##
===========================================
+ Coverage    62.16%   62.18%   +0.01%     
===========================================
  Files          212      212              
  Lines        17227    17278      +51     
===========================================
+ Hits         10710    10745      +35     
- Misses        5614     5626      +12     
- Partials       903      907       +4

Impacted Files	Coverage Δ
state/store.go	`70.07% <ø> (ø)`	⬆️
state/execution.go	`76.1% <100%> (+0.99%)`	⬆️
privval/tcp_server.go	`78.57% <0%> (-2.86%)`	⬇️
privval/ipc_server.go	`67.92% <0%> (-1.89%)`	⬇️
consensus/reactor.go	`67.2% <0%> (+0.92%)`	⬆️
libs/db/remotedb/remotedb.go	`41.52% <0%> (+4.68%)`	⬆️

melekes

🍇 🍷

CHANGELOG_PENDING.md

types/event_bus.go

kostko · 2018-11-09T14:40:53Z

All comments should be addressed.

melekes

💎

ebuchman

Not sure if this is the design we should go with - it feels a little adhoc to just add these tags to the NewBlock events. From looking at the fire event sequence for a block:

func fireEvents(logger log.Logger, eventBus types.BlockEventPublisher, block *types.Block, abciResponses *ABCIResponses) {
	eventBus.PublishEventNewBlock(types.EventDataNewBlock{block})
	eventBus.PublishEventNewBlockHeader(types.EventDataNewBlockHeader{block.Header})

	for i, tx := range block.Data.Txs {
		eventBus.PublishEventTx(types.EventDataTx{types.TxResult{
			Height: block.Height,
			Index:  uint32(i),
			Tx:     tx,
			Result: *(abciResponses.DeliverTx[i]),
		}})
	}

	abciValUpdates := abciResponses.EndBlock.ValidatorUpdates
	if len(abciValUpdates) > 0 {
		// if there were an error, we would've stopped in updateValidators
		updates, _ := types.PB2TM.ValidatorUpdates(abciValUpdates)
		eventBus.PublishEventValidatorSetUpdates(
			types.EventDataValidatorSetUpdates{ValidatorUpdates: updates})
	}
}

it feels more natural to follow the same order as the ABCI execution, by firing a BeginBlock event before, and an EndBlock event after, the set of tx events. Note we are already firing a ValidatorUpdates event after the txs, which could be subsumed by an EndBlock one.

Thoughts?

state/store.go

ebuchman · 2018-11-11T20:01:37Z

types/event_bus.go

+	}
+	return result
+}
+
 func (b *EventBus) PublishEventNewBlock(data EventDataNewBlock) error {


Would it maybe be better to have independent subscription events for BeginBlock and EndBlock, rather than stuffing them in with NewBlock/NewBlockHeader like this?

ebuchman · 2018-11-11T20:06:24Z

types/events.go

@@ -54,11 +55,17 @@ func RegisterEventDatas(cdc *amino.Codec) {

 type EventDataNewBlock struct {
 	Block *Block `json:"block"`
+
+	ResultBeginBlock abci.ResponseBeginBlock `json:"result_begin_block"`


Should we treat these like we treat transactions, as independent events, with EventDataBeginBlock and EventDataEndBlock?

What are the use cases for getting just this data - is it more useful with the block itself or with other results tags from the block?

kostko · 2018-11-14T10:11:08Z

Not sure if this is the design we should go with - it feels a little adhoc to just add these tags to the NewBlock events. From looking at the fire event sequence for a block:

Well, transaction tags are added to a transaction event, not to a separate event?

What would the hypothetical BeginBlock/EndBlock events contain? Only the ABCI responses? This seems strange, what if you (as a subscriber) want the block or the block header, then you need to maintain additional state?

This commit makes both EventNewBlock and EventNewBlockHeader emit tags on the event bus, so subscribers can use them in queries. This is a BREAKING change due to adding a field to the ABCIResponses structure which is persisted to disk.

ebuchman · 2018-11-16T15:59:46Z

Well, transaction tags are added to a transaction event, not to a separate event?

Right, they're part of the ResponseDeliverTx - I think it makes sense to have a single event per abci response.

What would the hypothetical BeginBlock/EndBlock events contain? Only the ABCI responses? This seems strange, what if you (as a subscriber) want the block or the block header, then you need to maintain additional state?

Note we have a validator update event - I think we should fold that into an EndBlock event, and we can include the tags there. If you really think it's necessary, we can put the header in the EndBlock event too, but that might be over kill.

We can avoid the BeginBlock event for now by putting its response in the NewBlock and NewBlockHeader events, like you've done. I think that makes sense.

My main gripe here is having the EndBlock event stuff distinct from the existing EventValidatorSetUpdates. I think we need to consolidate those.

ebuchman · 2018-11-16T16:04:04Z

My main gripe here is having the EndBlock event stuff distinct from the existing EventValidatorSetUpdates. I think we need to consolidate those.

Is that right? I'm not sure actually. Will some folks only want to subscribe to the ValidatorUpdates? Or is it fair to say that if you want to know about updates, subscribe to the EndBlock and check every time? I think it would be kind of nice for the Event system to as much as possible match the ABCI structure, unless there are really strong reasons otherwise.

We should probably write an ADR about all this ...

ebuchman · 2018-11-16T16:05:22Z

Last point: thanks for noting this is a breaking upgrade on the state. Appreciate it.

We can minimize the impact of that by writing a small upgrade tool that will allow people to upgrade their existing State. It can even be an automatic part of the node, now that we have the software version built into the state! Do you want to take this on?

Thanks!

kostko · 2018-11-16T17:37:22Z

We can minimize the impact of that by writing a small upgrade tool that will allow people to upgrade their existing State. It can even be an automatic part of the node, now that we have the software version built into the state! Do you want to take this on?

So I switched the field order as you suggested. Not sure how Amino serializes things, does it assign field tags based on field order? In this case the change may already be non-breaking now?

kostko · 2018-11-16T17:45:16Z

We can avoid the BeginBlock event for now by putting its response in the NewBlock and NewBlockHeader events, like you've done. I think that makes sense.

My main gripe here is having the EndBlock event stuff distinct from the existing EventValidatorSetUpdates. I think we need to consolidate those.

So the NewBlock/NewBlockHeader events would only contain responses and tags from BeginBlock and EventValidatorSetUpdates would only contain responses and tags from EndBlock?

For me, either would be fine actually as long as we can get the block height corresponding to the emitted tags together with the event(s).

@t-bast would this work for your use cases?

t-bast · 2018-11-16T17:59:46Z

Same for me, either would be fine. Thanks guys!

ebuchman

Ok, I think this is fine. Thanks all!

* develop: types: Emit tags from BeginBlock/EndBlock (tendermint#2747) Fix fast sync stack with wrong block tendermint#2457 (tendermint#2731) It's better read from genDoc than from state.validators when appHeight==0 in replay (tendermint#2893) update encoding spec (tendermint#2903) return back initially allowed level if we encounter allowed key (tendermint#2889) Handling integer IDs in JSON-RPC requests -- fixes tendermint#2366 (tendermint#2811) # Conflicts: # blockchain/reactor_test.go # blockchain/store_test.go # node/node_test.go # types/events.go

kostko requested review from ebuchman, melekes and xla as code owners November 2, 2018 09:30

kostko changed the base branch from master to develop November 2, 2018 09:42

kostko force-pushed the kostko/feature/beginblock-tags branch from 71fbcd7 to 4f293d9 Compare November 2, 2018 09:42

melekes reviewed Nov 3, 2018

View reviewed changes

state/execution.go Show resolved Hide resolved

kostko force-pushed the kostko/feature/beginblock-tags branch 2 times, most recently from 78c16ef to 8d15671 Compare November 5, 2018 10:02

kostko changed the title ~~types: Emit tags from BeginBlock/EndBlock with EventNewBlock~~ types: Emit tags from BeginBlock/EndBlock Nov 5, 2018

melekes reviewed Nov 5, 2018

View reviewed changes

kostko force-pushed the kostko/feature/beginblock-tags branch 3 times, most recently from 317353b to 441a0f4 Compare November 9, 2018 08:52

kostko requested a review from zramsay as a code owner November 9, 2018 08:52

melekes reviewed Nov 9, 2018

View reviewed changes

CHANGELOG_PENDING.md Outdated Show resolved Hide resolved

types/event_bus.go Outdated Show resolved Hide resolved

types/event_bus.go Outdated Show resolved Hide resolved

types/event_bus.go Show resolved Hide resolved

kostko force-pushed the kostko/feature/beginblock-tags branch from 441a0f4 to ea14968 Compare November 9, 2018 09:23

melekes approved these changes Nov 9, 2018

View reviewed changes

ebuchman reviewed Nov 11, 2018

View reviewed changes

kostko force-pushed the kostko/feature/beginblock-tags branch from ea14968 to 2564ec0 Compare November 14, 2018 10:30

types: Emit tags from BeginBlock/EndBlock

b7aa527

This commit makes both EventNewBlock and EventNewBlockHeader emit tags on the event bus, so subscribers can use them in queries. This is a BREAKING change due to adding a field to the ABCIResponses structure which is persisted to disk.

kostko force-pushed the kostko/feature/beginblock-tags branch from 2564ec0 to b7aa527 Compare November 16, 2018 08:10

ebuchman mentioned this pull request Nov 26, 2018

Stuck in fast sync with no peers and validator set > 1 #2916

Closed

ebuchman approved these changes Nov 26, 2018

View reviewed changes

Merge branch 'develop' into kostko/feature/beginblock-tags

e91fb31

ebuchman merged commit 99b9c9b into tendermint:develop Nov 27, 2018

kostko mentioned this pull request Nov 27, 2018

Bump tendermint to 0.27 oasisprotocol/oasis-core#1158

Closed

ebuchman mentioned this pull request Mar 26, 2019

rpc: subscribe to begin/endblock tags #1571

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

types: Emit tags from BeginBlock/EndBlock #2747

types: Emit tags from BeginBlock/EndBlock #2747

kostko commented Nov 2, 2018 •

edited

melekes left a comment

kostko commented Nov 5, 2018

kostko commented Nov 5, 2018

melekes left a comment

kostko commented Nov 9, 2018

codecov-io commented Nov 9, 2018 •

edited

melekes left a comment

kostko commented Nov 9, 2018

melekes left a comment

ebuchman left a comment

ebuchman Nov 11, 2018

ebuchman Nov 11, 2018

kostko commented Nov 14, 2018

ebuchman commented Nov 16, 2018 •

edited

ebuchman commented Nov 16, 2018

ebuchman commented Nov 16, 2018

kostko commented Nov 16, 2018

kostko commented Nov 16, 2018

t-bast commented Nov 16, 2018

ebuchman left a comment •

edited

types: Emit tags from BeginBlock/EndBlock #2747

types: Emit tags from BeginBlock/EndBlock #2747

Conversation

kostko commented Nov 2, 2018 • edited

melekes left a comment

Choose a reason for hiding this comment

kostko commented Nov 5, 2018

kostko commented Nov 5, 2018

melekes left a comment

Choose a reason for hiding this comment

kostko commented Nov 9, 2018

codecov-io commented Nov 9, 2018 • edited

Codecov Report

melekes left a comment

Choose a reason for hiding this comment

kostko commented Nov 9, 2018

melekes left a comment

Choose a reason for hiding this comment

ebuchman left a comment

Choose a reason for hiding this comment

ebuchman Nov 11, 2018

Choose a reason for hiding this comment

ebuchman Nov 11, 2018

Choose a reason for hiding this comment

kostko commented Nov 14, 2018

ebuchman commented Nov 16, 2018 • edited

ebuchman commented Nov 16, 2018

ebuchman commented Nov 16, 2018

kostko commented Nov 16, 2018

kostko commented Nov 16, 2018

t-bast commented Nov 16, 2018

ebuchman left a comment • edited

Choose a reason for hiding this comment

kostko commented Nov 2, 2018 •

edited

codecov-io commented Nov 9, 2018 •

edited

ebuchman commented Nov 16, 2018 •

edited

ebuchman left a comment •

edited