Remove invalid transactions from PrepareRequest payload#984
Remove invalid transactions from PrepareRequest payload#984superboyiii merged 4 commits intoneo-project:master-n3from
Conversation
Pull Request Test Coverage Report for Build 21316142171Details
💛 - Coveralls |
roman-khimov
left a comment
roman-khimov
left a comment
There was a problem hiding this comment.
- Consistent policy configuration can be achieved via Policy contract.
- Node disputes are to be resolved via CV, some other node will propose something acceptable. Bad primaries are to de-elected.
- Nodes directly telling other nodes what's right or wrong doesn't seem reliable, we need BFT number of them following the protocol, that's it.
But currently they will change view forever and de-election is not something automatically, require vote changing |
Why? Node A proposes "bad" tx T, CV happens, node B proposes another set of transactions without T, everyone agrees (no policy conflict), block produced. |
|
If the transaction is still in the memory pool, A will submit this transaction in each round. |
Yes. And it's not a problem to me. This (incorrect Primary) can be discovered quickly and either configuration will be fixed or node will be voted out. Otherwise policies are for Policy contract, it has (almost) all the mechanisms for that. |
|
I would not call it "incorrect." The policy for each node may differ and may not necessarily be correct or incorrect. Therefore, we need a mechanism to intersect the policy of each node when more than F nodes are inconsistent, in order to enhance the stability of the consensus protocol. |
As long as BFT number of nodes agree we're OK. If we're to imagine each node doing local policing of any kind we're in trouble and this PR can't fix it. A node can have a policy of always using 42 as block nonce and rejecting any other proposals for example.
The mechanism for that is CV. Just like in any other case where agreement can't be reach for the current proposal (or proposal is missing). |
| { | ||
| case ChangeViewReason.TxRejectedByPolicy: | ||
| case ChangeViewReason.TxInvalid: | ||
| RejectedHashes = reader.ReadSerializableArray<UInt256>(ushort.MaxValue); |
There was a problem hiding this comment.
If it's the end of the stream we should deserialize as empty, or we will be incompatible with other nodes (hardfork)
There was a problem hiding this comment.
Currently, there is no way to determine if the MemoryReader has reached the end of the data. Modifying the MemoryReader would require releasing a new version of Neo. Therefore, we will not make any changes for now and will release the update with the next version. This only involves modifications to the consensus protocol and will not result in a hard fork.
vncoelho
left a comment
vncoelho
left a comment
There was a problem hiding this comment.
This is related to neo-project/neo#4333
It is needed
| // Iterate transaction until reach the size or maximum system fee | ||
| foreach (Transaction tx in txs) | ||
| { | ||
| if (InvalidTransactions.TryGetValue(tx.Hash, out var hashset)) |
There was a problem hiding this comment.
Add some comment
// Verify if there is, at least, one honest node that consider this TX as invalid. If so, skip it.
There was a problem hiding this comment.
One is not enough; at least more than F are needed.
8 participants
If the policy configurations among consensus nodes are inconsistent (e.g.,
MaxBlockSizeorMaxBlockSystemFee), it may lead to some invalid transactions being persistently retained in some consensus nodes, preventing block creation. A mechanism is needed to ensure that when a transaction is identified as invalid by more thanFconsensus nodes, all consensus nodes will refrain from including that transaction in thePrepareRequestpayload.