[AnnaShaleva]

1. Explicitly ask the user not to modify the list of transactions and the map of missing hashes got from (PrepareRequest).Transactions(). Modification will lead to improper library functioning since dBFT relies on the same map in (*Context).hasAllTransactions(). Ref. consensus: migrate to extended PrepareRequest format bane-labs/go-ethereum#655 (comment).
2. Forbid watch-only nodes to request missing transactions. Processing of missing transactions is also not supported by watch-only nodes.

[codecov]

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 62.09%. Comparing base (baa1d86) to head (63e6948).

Files with missing lines	Patch %	Lines
dbft.go	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #169      +/-   ##
==========================================
- Coverage   62.11%   62.09%   -0.02%     
==========================================
  Files          32       32              
  Lines        1560     1562       +2     
==========================================
+ Hits          969      970       +1     
- Misses        590      591       +1     
  Partials        1        1              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[AnnaShaleva]

@roman-khimov if we'd like to preserve the old behaviour (backup node is not able to construct block in case if some transactions are missing) then the first commit is enough.

On the other hand, if all transactions are present in the watch-only's mempool then it's able to advance state machine and move forward with it's own local block construction (according to usages of (*Context).CreatePreBlock), e.g.:

dbft/check.go

Line 70 in baa1d86

d.preBlock = d.CreatePreBlock()

From that point, watch-only may be allowed to process missing transactions the same way as proper CN nodes. If so, then I'd suggest to replace this condition:

dbft/dbft.go

Lines 176 to 180 in baa1d86

	if !d.IsBackup() || d.NotAcceptingPayloadsDueToViewChanging() ||
	!d.RequestSentOrReceived() || d.ResponseSent() || d.PreCommitSent() ||
	d.CommitSent() || d.BlockSent() || d.hasAllTransactions() {
	return
	}

by the following one:

	if !(d.IsBackup() || d.Context.WatchOnly()) || d.NotAcceptingPayloadsDueToViewChanging() ||
		!d.RequestSentOrReceived() || d.ResponseSent() || d.PreCommitSent() ||
		d.CommitSent() || d.BlockSent() || d.hasAllTransactions() {
		return
	}

(d *DBFT[H, Tx]) addTransaction itself is able to handle the case of watch-only nodes:

dbft/dbft.go

Lines 61 to 63 in baa1d86

	if d.IsPrimary() || d.Context.WatchOnly() {
	return
	}

But still, it's a library behaviour change. Implemented in the second commit, ACK?

[roman-khimov]

I'd say that watch-only shouldn't request for missing transactions. It'll be almost irrelevant with Iara anyway and I'd expect watch-only to be passive, not actively trying to create a block (even though it can). Practically watch-only is two cases:

• committee node that has a key, but is not a validator
• some monitoring node running for fun

Both can just log things happening, but both don't need to construct/check a block. And not doing it seems simpler.

[AnnaShaleva]

Now watch-only node is not allowed neither process nor request missing transactions, so if something is missing then it'll stuck at the intermediate consensus state until block is received through the network.

[AnnaShaleva]

Just for the record, C# watch-only node requests missing transactions: https://github.com/neo-project/neo-node/blob/ccd6a4bb09bae3efc4c2a4b86043b3415ad6cbd9/plugins/DBFTPlugin/Consensus/ConsensusService.OnMessage.cs#L177, so we'll have a difference in behaviour.