removed the request from the pool if the the proposal is bad

[pfi79]

In the documentation and in the examples, the channel update transaction is send to one and only one orderer. Like in raft. The smartbft features are not used.

I checked what happens if a channel update transaction is sent to each orderer. There are errors.

What's happening?
When an orderer receives a channel update transaction, it takes the current channel config and applies the update to it and re-signs the transaction. It then sends it to the smartbft consensus. Total 7 different transactions appear in the consensus (1 error - not critical).

Then, one transaction (of the leader) is accepted and the rest automatically become bad. Transactions that followers send to the leader do not reach him, as they are considered bad. Followers change the leader. The new leader generates a proposal that contains a bad transaction. This proposal returns an error when checked. The mechanism of leader change is started. So in an infinite loop (the 2nd error is critical).

My current solution fixes the 2nd error. In case there is a bad transaction in the proposal, we remove it from the request pool.

[yacovm]

Nice catch, but this is not the way to fix it.

SmartBFT interprets that config change as different transactions probably due to the signatures being different.

The way to fix it, is to explicitly handle this case upon a commit of a config block:

1. In the chain's deliver function
2. When we replicate a config block in the synchronizer.

If a config block is committed, we must do a deep scan of the request pool and purge every transaction that has the same config update payload as the one we just committed, ignoring the signature.

[pfi79]

Nice catch, but this is not the way to fix it.

SmartBFT interprets that config change as different transactions probably due to the signatures being different.

The way to fix it, is to explicitly handle this case upon a commit of a config block:

1. In the chain's deliver function
2. When we replicate a config block in the synchronizer.

If a config block is committed, we must do a deep scan of the request pool and purge every transaction that has the same config update payload as the one we just committed, ignoring the signature.

Let me put it this way, the situation I am correcting:
If someone comes up with a way to add a transaction to the request pool that becomes bad while there. We'll just delete it.

Just because this particular option has been found now doesn't mean that there won't be others in the future.

For example, I will prepare 4 different transactions for 4 orders, which you will not define as the same.
For example there are 5 config parameters I will distribute their change like this:
1 transaction changes 1 and 5 parameters
2 transaction changes 2 and 5 parameters
3 transaction changes 3 and 5 parameters
4 transaction changes 4 and 5 parameters.

I'll send to each node at the same time.
And repeat the same thing.

[pfi79]

2. When we replicate a config block in the synchronizer.

Forgot to mention.
In this case, the block with the config is not synchronised. All nodes took part in its delivery, all of them installed it for themselves.

Any attempts to synchronise return the response that you have the latest everything

[yacovm]

But the only way to change the validity of a transaction for an orderer is via a config block. Therefore, config blocks is what we should be concerned about.

We need to also take in account synchronization, because a node might be behind and in that case it will not participate in consensus, so the only way to notify that a config block was committed, is via the synchronization code.

Transaction validation in BFT happens at the pre-prepare processing phase, that is not the right spot to remove requests from the request pool.

Requests should only be removed from the request pool upon a block commit.

[pfi79]

But the only way to change the validity of a transaction for an orderer is via a config block. Therefore, config blocks is what we should be concerned about.

I have already shown above that I can drop the channel and unequal config updates

For example, I will prepare 4 different transactions for 4 orders, which you will not define as the same.
For example there are 5 config parameters I will distribute their change like this:
1 transaction changes 1 and 5 parameters
2 transaction changes 2 and 5 parameters
3 transaction changes 3 and 5 parameters
4 transaction changes 4 and 5 parameters.

I'll send to each node at the same time.

We need to also take in account synchronization, because a node might be behind and in that case it will not participate in consensus, so the only way to notify that a config block was committed, is via the synchronization code.

Let me remind you that synchronization will fix all this with the c.MaybePruneRevokedRequests() method.
In my case there is no synchronization with lag.

		case <-c.syncChan:
			c.Logger.Debugf("get msg from syncChan")
			view, seq, dec := c.sync()
			c.MaybePruneRevokedRequests()
                .......

func (c *Controller) MaybePruneRevokedRequests() {
	oldVerSqn := c.verificationSequence
	newVerSqn := c.Verifier.VerificationSequence()
	if newVerSqn == oldVerSqn {
		return
	}
	c.verificationSequence = newVerSqn

	c.Logger.Infof("Verification sequence changed: %d --> %d", oldVerSqn, newVerSqn)
	c.RequestPool.Prune(func(req []byte) error {
		_, err := c.Verifier.VerifyRequest(req)
		return err
	})
         .................

But verificationSequences are equal, so it doesn't get to deletion.

Transaction validation in BFT happens at the pre-prepare processing phase, that is not the right spot to remove requests from the request pool.
Requests should only be removed from the request pool upon a block commit.

1. In my case, it's not getting to the commit phase. And it never will. Consensus ends at the pre-prepare processing phase.
   That's it.
2. So you're suggesting we delete it when it "never comes"?

[pfi79]

@yacovm I guess I should add an explanation:

I first wanted to change c.MaybePruneRevokedRequests by removing the check for equal verificationSequence. But I thought it would be too expensive to search the requests pool for each sync.

Then I thought that during synchronisation in fabric to call deletion from the pool of invalid transactions, but how to pass in synchronisation context to call deletion not every synchronisation.

And the clearest, easiest and cheapest solution was to remove from the pool after a failed proposal check. Anyway, this option works. Probably the best. But when a better option is found, you can always remove mine and add a better one.