Restrict maximum number of conflicting transactions per Ledger's conflict record #2911
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts commit 2a69047. As it was said in neo-project#2909 (comment), we can't save only a part of on-chained conflicting signers. Instead, we need to construct new blocks in such way so that there were no conflicting signers limit overflow. It's not hard to implement, see the further commits. Signed-off-by: Anna Shaleva <[email protected]>
Instead of restricting the maximum number of conflicting signers per conflict record we can restrict the number of transactions that make their contribution to this conflict record. The constraint nature is the same, it doesn't allow to spam the chain with a large set of transactions with the same Conflicts attribute. However, this approach has several benefits over restricting the number of signers for these conflicting transactions: 1. It significantly simplifies and accelerates a newly-pooled transactions verification. We don't need to perform this `Concat(signers).Distinct().Count()` operation anymore on new transaction addition. 2. It has a space for improvement which is not presented in this commit, but have to be implemented if the current solution will be accepted by the core developers. A space for improvement is the following: during Conflicts attribute verification we don't actually need to retrieve the whole list of conflicting signers for the conflict record. What we need instead is to check the `ConflictingTransactionsCount` field only. Thus, we may safely omit the `ConflictingSigners` deserialisation from stack item. 3. It allows to easily port the same constraint to the mempool (see the further commit) which, in turn, roughly restrict the overall possible number of transactions per conflict record in Ledger contract up to 2*MaxAllowedConflictingTransactions. It means, that the maximum number of signers per conflict record in the worst case equals to 2*MaxAllowedConflictingTransactions*(MaxTransactionAttributes-1)=480. At the same time, the overhead the current commit brings in the `TransactionState` is quite insignificant (it's only a single extra Integer field). Note 1: this PR changes the native Ledger scheme, thus, requires the DB resync on upgrade. States will differ from the current 3.6.0 T5. Note 2: this PR (as far as neo-project#2908) doesn't help with those transactions that already were included into blocks 2690038-2690040 of the current T5 due to the neo-project#2907 (comment). We need to have a separate discussion on these "malicious" blocks and decide how to handle them. However, this PR doesn't ever prevent the node from the proper processing of these blocks on resync, the blocks will be processed successfully (with HALT state, the same as they were processed by the 3.6.0 release). Signed-off-by: Anna Shaleva <[email protected]>
Port the Ledger conflicting tx restriction logic to the MemoryPool: the maximum number of mempooled transactions that has the same Conflicts attribute (i.e. those that conflict with the same transaction) is restricted by the LedgerContract.MaxAllowedConflictingTransactions constant. This restriction, combined with the existing Ledger's storage restriction, roughly restricts the overall possible number of transactions per conflict record in Ledger contract up to 2*MaxAllowedConflictingTransactions. It means, that the maximum number of signers per conflict record in the worst case equals to 2*MaxAllowedConflictingTransactions*(MaxTransactionAttributes-1)=480. Signed-off-by: Anna Shaleva <[email protected]>
This was referenced Sep 17, 2023
shargon
reviewed
Sep 18, 2023
| return false; | ||
|
|
||
| // Hash duplicated on different attributes | ||
| if (!hashes.Add(attr.Hash)) |
There was a problem hiding this comment.
@AnnaShaleva I think that we should avoid hashes repetitions
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TL;DR
Please, pay attention that the target branch of this PR is not
master, it'smax-conflicts(#2908). This PR contains a minor improvement for the native Ledger conflict records fix originally implemented in the #2908. It also contains related memory pool fix (the memory pool problem is described in #2908 (comment)).To core devs
This PR is ready to be reviewed and aimed (after its merge into #2908) to fix #2907. However, we have another approach that should fix the #2907 and that is even better than approach presented in the current PR and in #2908. I will publish the new approach and motivation in a separate commit for further discussions. We (Roman and me) are kindly asking the core devs to carefully consider the new approach before merging this PR and before merging #2908 (it won't take a lot of time).
Update: the new approach is described in #2907 (comment).
Full PR description
The minor improvement this PR contributes to #2908 is: Instead of restricting the maximum number of conflicting signers per conflict record we can restrict the number of transactions that make their contribution to this conflict record. The constraint nature is the same, it doesn't allow to spam the chain with a large set of transactions that have identical Conflicts attribute. This approach has several benefits over restricting the number of signers for these conflicting transactions:
Concat(signers).Distinct().Count()operation anymore on new transaction addition to check on-chain conflicts.ConflictingTransactionsCountfield only. Thus, we may safely omit theConflictingSignersdeserialisation from stack item.MaxAllowedConflictingTransactions. It means that the maximum number of signers per conflict record in the worst case equals to 2xMaxAllowedConflictingTransactionsx(MaxTransactionAttributes-1)=480.At the same time, the overhead the presented approach brings in the
TransactionStateand overall DB size is quite insignificant (it's only a single additional Integer field per one conflict record that should be serialized).The mempool fix that is presented in this PR solves the problem described in #2908 (comment). The memory pool fix itself is a port of the Ledger conflicting tx restriction logic to the MemoryPool: the maximum number of mempooled transactions that has the same Conflicts attribute (i.e. those transactions that conflict with the same transaction) is restricted by the LedgerContract.MaxAllowedConflictingTransactions constant. This restriction, combined with the existing Ledger's storage restriction, roughly restricts the overall possible number of transactions per conflict record in Ledger contract up to 2*MaxAllowedConflictingTransactions at max. It means that the maximum number of signers per conflict record in the worst case equals to 2x
MaxAllowedConflictingTransactionsx(MaxTransactionAttributes-1)=480 (with the current values of these constants).Caveats
Note 1: this PR changes the native Ledger scheme, thus, requires the DB resync on upgrade. States will differ from the current 3.6.0 T5.
Note 2: this PR (as far as #2908) doesn't help with those transactions that already were included into blocks 2690038-2690040 of the current T5 due to the #2907 (comment). We need to have a separate discussion on these "malicious" blocks and decide how to handle them. However, this PR doesn't ever prevent the node from the proper processing of these blocks on resync, the blocks will be processed successfully (with HALT state, i.e. the same way as they were processed by the 3.6.0 release).