Do we want to group *AnnotatedBlock and *RoundResults in dedicated struct? Also use it during Commit and in other places. No strong opinion.

Also, the following git messages are not in correct format:

• go/consensus/cometbft/roothash: return 0 value on error
• go/roothash: Batch writting runtime blocks during reindex

I would be more general and remove values 1000 and 2x from the description. I would only say that blocks and results are written in batches.

Which context is the right one, ctx or sc.ctx?

My understanding is they are the same. I have improved this in 14e21df to always pass context explicitly. Technically WatchBlocks and WatchEvents methods still use sc.Ctx and simply ignore the context they receive (so kinda misleading for the caller :/).

This is existing code so would prefer to tackle this problem elsewhere unless trivial. Any suggestions what to do in such case? Probably we want to combine two contexts and propagate correct error, or stop passing it if find it unnecessarily.

Any suggestions what to do in such case?

Never store context in a struct, and if possible, when making changes to the existing code fix functions so that context is the first argument.

This method is public, so I can use it wherever I want, not only during reindex. Fix the comment.

Too long.

Are we planning to call CommitBatch also in parallel? Will this work? I think that currently we cannot call Commit in parallel, or can we?
Must blocks be in order, sorted by round?
Must this be called in order, sorted by round?
What happens if I call Commit, CommitBatch, Commit, ... Will things still work?
Why don't we notify if needed?

You can call it in parallel see benchmarks. Notice that parallelization of consensus blocks without batching (see benchmarks), only improved performance by 2x. This is the performance gain we get with batching only (still running things sequentially). It is for this reason that I would prefer to get in batching as a separate/independent feature.

Must blocks be in order, sorted by round?
Must this be called in order, sorted by round?

I should elaborate these requirements in the docstring. Thanks for pointing.

What happens if I call Commit, CommitBatch, Commit, ... Will things still work?

It should work, I am writting a test for this rn :).

Why don't we notify if needed?

Because this will be only used during reindex and we don't notify during it. If someone will later need this functionality I suggest it is added at that point (should be trivial). Agree that comment should probably changed to something simpler/with less assumptions:

// This method does not notify watchers.

You can call it in parallel

No you can't, because the meta will be incorrect.

It should work, I am writting a test for this rn :).

I would say not, because Commit has extra code, which CommitBatch doesn't.

No you can't, because the meta will be incorrect.

You are right. I think we should extend the batching to support that when/if we start calling it in parallel.

I would say not, because Commit has extra code, which CommitBatch doesn't.

You are right, we are forgetting to notify pruner. Will fix this. As for the notify I still think we don't have to expose this functionality. See existing Commit docstring/promise:

	// Commit commits an annotated block into history. If notify is set to true,
	// the watchers will be notified about the new block. Disable notify when
	// doing reindexing.

I think we should extend the batching to support that when/if we start calling it in parallel.

As desribed in Possible_solutions/blockers/2, this should be fixed, by refactoring block history to operate in two modes; reindexing and reindexed. During reindexing, metadata will be ingnored, and we won't notify watchers expect for the latest round of reindex (as we do rn).

Technically, you can already safely call Commit and CommitBatch in parallel, however you will receive an error (which you should handle) in most cases, due to metadata mismatch.

Alternative, as discussed on the slack is to try to parallelize reading batch part (update) only (as suggested by @ptrus), and skip this complex refactor altogether, provided we will be happy with performance.

I would say not, because Commit has extra code, which CommitBatch doesn't.

I have fixed this following your suggestion in private: where Commit becomes a helper for CommitBatch of single entry. I have added separate commit for that.

Prior to that I have also extended TestWatchBlocks corner cases to capture existing behaviour: see commit.

Docstrings have also been reworked for both Commit and CommitBatch.

Finally, as discussed in private we will only fetch consensus state in parallel. The writing part will be written sequentially (in batches).

I would say not, because Commit has extra code, which CommitBatch doesn't.

I have fixed this following your suggestion in private: where Commit becomes a helper for CommitBatch of single entry. I have added separate commit for that.

Prior to that I have also extended TestWatchBlocks corner cases to capture existing behaviour: see commit.

Docstrings have also been reworked for both Commit and CommitBatch.

You don't need to describe these changes in detail. I will see that in the code. Just extra work for you and for reviewers.

Finally, as discussed in private we will only fetch consensus state in parallel. The writing part will be written sequentially (in batches).

This comment is fine, to let other reviewers know what we discussed.

Transactions should always be fast and small. Therefore, you should improve this by moving outside of the tx data encoding, data verification, and calculation of max/min.

Fixed here eae4b93, please let me know if you see simpler way of doing it?

Not sure it would have much impact though since badger uses (serializable) snapshot isolation, so reads are actually never blocked by writes (?). We will not support writing in parallel, due to metadata mismatch (and writing part not being a bottleneck).

We should probably still pre-process (pre-validate) it to follow best practices and not to make assumptions how code will be used in the future?

I was thinking, if we have prunning set to only retain few blocks (possibly one), or the time interval is very small, could batching always receive an error? Let's say on my machine to process a batch it takes 200-300ms. Would it be better to defensively check what is the last retained round again if we receive an error, and simply update the latest height if it has changed?

cc @ptrus

I think the minimum interval is 1 second currently, and in practice it's likely much more (e.g. in minutes).

Likely not problematic in practice, so I don't really have strong opinions on how to handle it.

As discussed with @ptrus it might be a good idea to harden existing history reindex e2e test, by possibly adding a query for every round that we expect it should be reindexed.

Finally also in long term e2e tests we are also querying old random blocks && restarting nodes in between possibly triggering reindex?

Maybe create a follow-up issue for this. I think the simplest way would be running a TestClientScenario which queries (runs GetKeyValueTx) queries all the old blocks, at the end of the existing history reindex e2e test.

Agree, #6070 created to track this.

Maybe

to make it a bit more descriptive.

I believe reindex returns api.RoundInvalid and no error, only when there was no runtime rounds finalized in the contained batch of heights, lets update the comment to better reflect this:
Something like

Commit is now only used when not doing history reindex right? I believe in that case we always want to notify? Is there a case where notify would be false with the new code?
If not, we should maybe just remove the argument and always notify.

Is CommitBatch ever needed with notify being true? I would expect not, so maybe lets remove the argument and never notify from CommitBatch?

Makes sense also for the Commit part.

Done here 089fd7d. If we like I will squash it.

cc @peternose ?

I guess so, since we need to review from the begging anyway.

I think I have found a minor issue inside master related to this.

Context:
We always notify blocks when processFinalizedEvent succeeds. Either normal event processing or successful history reindex. The first time this happens is after first reindex, which also marks common committee node as initialized (see).

Rn, after first reindex, there is always a second reindex. Finally in case of any error during normal event processing, this simply marks a block history for reindex.

Problem:
Client committee node waits for initialization to be finished (see). Afterwards, it subscribes to runtime history WatchBlocks (see), thinking it will receive all blocks from this point on. Since we don't notify during reindex, in extremely unlikely scenario, SubmitTx may be called during subsequent reindex, meaning client worker may never receive blocks for the corresponding transaction, thus never handling SubmitTx properly.

** Observe we have two different WatchBlocks methods: 1. and 2. Common committee node is subscribed to first, common client committee node to second.

Solution:

• Do only one initial reindex, repeat until n blocks from latest height.
  • If later normal events processing fails, all subsequent reindexes should notify about the blocks that happen in between...
• Alternatively handle this in client committee worker, by e.g. fetching all missing blocks manually (not best).

If you agree this is problematic I would prefer to tackle this as a follow-up given this is existing (separate issue) that we have. We may want to preserve notify in the CommitBatch if you think first bullet is the right solution?

Actually this is not a problem, since if you submit transaction in the middle of n-th reindex (unlikely), this is already past and transaction can only be processed when you catch-up, so you are guaranteed to receive this block after-all...?

It makes more sense to do this check above, right after the existing length checks, before doing any array initializations.

It's reasonable to expect that roothash.AnnotatedBlock always has Block field present (unless the API docs explicitly mention it could be null), and panicking in case it does not is probably desired (since something major must be wrong, likely a developer error), so no need for this check.

Same for below in the loop.

We don't check this in other cases, e.g.:

Makes sense. It should be resolved now.

It's reasonable to expect that roothash.AnnotatedBlock always has Block field present (unless the API docs explicitly mention it could be null)

Yes, this is the best practice 👍 A field cannot be nil, unless explicitly stated. And if it can be nil, try to change the code (if possible) so that it can't be.

I think the minimum interval is 1 second currently, and in practice it's likely much more (e.g. in minutes).

Likely not problematic in practice, so I don't really have strong opinions on how to handle it.

This won't work if we have multiple runtime blocks per one consensus block in the future. So why adding this limitation?

I would put these two ifs under another if meta.LastConsensusHeight != 0 {.

I was trying to prevent receiving multiple rounds for the same height in batch. I see your point that this limits future extension, albeit I should probably still validate for this rn? cc @ptrus

No, I think that previous implementation took into account that multiple blocks can be published on the same height. And now you are throwing that away. In general, I would stay that we shouldn't worry about consensus block heights at all, but didn't think this through. In theory, there could be a runtime which publishes runtime block 2 in height 1 and block 1 in height 2.

Now, we don't log sc.logger.Debug("commit block", for every commit.

This message could be improved, so that error reads like meaningful sentence and not an equation.

What is node, what is local storage from API point of view? Those two things should be unknwon.

We have existing references to this:

I see your point though.

If I checked correctly, there is no function in the API of the history which uses or mentions nodes or local storage. So giving a comment for a special use case which we use, is not the best.

This could be wrapped in a subtest so that the name tells you what you are testing.

It is nice to test the happy path first, unless that complicates things a lot.

You can create a new commit for this, so that we can revert if needed. Can you test speed with and without these changes, just to be sure we don't slower things.

Would this be also ok?

func (d *DB) commitBatch(blks []*roothash.AnnotatedBlock, results []*roothash.RoundResults) error {
	if len(blks) != len(results) {
		return fmt.Errorf("%d blocks != %d round results", len(blks), len(results))
	}

	batchSize := len(blks)
	if batchSize == 0 {
		return nil
	}

	// Transaction should be fast, therefore we pre-process encoding and validation.
	blkKeys := make([][]byte, batchSize)
	blkVals := make([][]byte, batchSize)
	resKeys := make([][]byte, batchSize)
	resVals := make([][]byte, batchSize)

	var lastHeight int64
	var lastRound uint64

	for i := 0; i < batchSize; i++ {
		if rtID := blks[i].Block.Header.Namespace; !rtID.Equal(&d.runtimeID) {
			return fmt.Errorf("runtime mismatch, want %s, got %s)", d.runtimeID, rtID)
		}

		height := blks[i].Height
		if height <= lastHeight {
			return fmt.Errorf("out-of-order block (height: %d came after %d)", height, lastHeight)
		}

		round := blks[i].Block.Header.Round
		if round <= lastRound {
			return fmt.Errorf("out-of-order block (round: %d came after %d)", round, lastRound)
		}

		lastHeight = height
		lastRound = round

		blkKeys[i], blkVals[i] = blockKeyFmt.Encode(round), cbor.Marshal(blks[i])
		resKeys[i], resVals[i] = roundResultsKeyFmt.Encode(round), cbor.Marshal(results[i])
	}

	// Commit batch transaction.
	return d.db.Update(func(tx *badger.Txn) error {
		meta, err := d.queryGetMetadata(tx)
		if err != nil {
			return err
		}

		if meta.LastConsensusHeight != 0 {
			if firstHeight := blks[0].Height; firstHeight <= meta.LastConsensusHeight {
				return fmt.Errorf("commit at lower or equal consensus height (current: %d wanted: %d)",
					meta.LastConsensusHeight,
					firstHeight,
				)
			}

			if firstRound := blks[0].Block.Header.Round; firstRound <= meta.LastRound {
				return fmt.Errorf("commit at lower or equal round (current: %d wanted: %d)",
					meta.LastRound,
					firstRound,
				)
			}
		}

		for i := 0; i < batchSize; i++ {
			if err = tx.Set(blkKeys[i], blkVals[i]); err != nil {
				return err
			}
			if err = tx.Set(resKeys[i], resVals[i]); err != nil {
				return err
			}
		}

		meta.LastRound = lastRound
		meta.LastConsensusHeight = lastHeight

		return tx.Set(metadataKeyFmt.Encode(), cbor.Marshal(meta))
	})
}

Will bench this tmr, albeit I don't expect major slowdowns here.

I have already benchmarked sorting every batch, before committing, since later when we will fetch data in parallel we will have to sort items first. Luckily it was negligible.

I tested this locally with artificial data and it looked like optimization (similar to the one above) was slower. Have no idea why. So I would test this before committing. But in general, it should still hold that locks should be released as fast as possible.

This comment is not the best.

This is very strange. How will watcher react if someone calls Commit, CommitBatch, and Commit. Where are the missing blocks? We should find a neater solution as once database is available for use/watching, all blocks should be reported.