Adding gas and byte size limits to collections #91
Conversation
ramtinms
changed the title
Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>
module/builder/collection/config.go
Outdated
| @@ -28,6 +28,12 @@ type Config struct { | |||
| // UnlimitedPayer is a set of addresses which are not affected by per-payer | |||
| // rate limiting. | |||
| UnlimitedPayers map[flow.Address]struct{} | |||
|
|
|||
| // MaxCollectionByteSize is the maximum byte size of a collection. | |||
| MaxCollectionByteSize uint | |||
There was a problem hiding this comment.
any reason why one is uint and other uint64?
There was a problem hiding this comment.
not really, I think anyway is better to avoid uint in case we run the protocol on 32bits machines in the future.
There was a problem hiding this comment.
I changed it to uint64 👍
| log = log.With(). | ||
| Hex("local_cluster", logging.ID(localCluster.Fingerprint())). | ||
| Hex("tx_cluster", logging.ID(txCluster.Fingerprint())). | ||
| Hex("local_cluster", logging.ID(localClusterFingerPrint)). |
There was a problem hiding this comment.
is fingerprint an ID? I understand that this function, AFAIR just hexprints it, but seems semantically wrong to use ID() on fingerprint
There was a problem hiding this comment.
I think fingerprint is different than ID here as it ignores some data parts I guess, but since I don't know that much about this part of the code, I won't change it right now.
There was a problem hiding this comment.
I see your point on semantics. the issue is we don't have a fingerprint type, and it still returns an Identifier type
| @@ -36,6 +42,8 @@ func DefaultConfig() Config { | |||
| ExpiryBuffer: 15, // 15 blocks for collections to be included | |||
| MaxPayerTransactionRate: 0, // no rate limiting | |||
| UnlimitedPayers: make(map[flow.Address]struct{}), // no unlimited payers | |||
| MaxCollectionByteSize: 1000000, // 1MB | |||
| MaxCollectionTotalGas: uint64(1000000), // 1M | |||
There was a problem hiding this comment.
The limit has been defined there, why defining again here?
Could we let me refer to the same number?
I'm afraid one day we change one place and forgot to change the other.
There was a problem hiding this comment.
I just followed the convention in the code to have a DefaultConfig and command line support to change it if needed. I won't change it as it needs to change all other values as well. maybe something for the future to change, cc: @jordanschalm
| for _, tx := range b.transactions.All() { | ||
|
|
||
| // if we have reached maximum number of transactions, stop | ||
| if uint(len(transactions)) >= b.config.MaxCollectionSize { | ||
| break | ||
| } | ||
|
|
||
| // we can break cause | ||
| // max byte size per tx << the max collection byte size | ||
| // to make it more effective in the future we can continue adding smaller ones |
There was a problem hiding this comment.
I'm concerned by this comment:
In the worst case, if the byte size of the first transaction returned by transactions.All() is bigger than the collection's max byte size, then no transaction will be added. The leader will keep proposing empty collections.
Could we add a check to say if the single transaction's byte size is bigger than the max colleciton byte size, we will continue instead of break:
if tx.ByteSize() > b.config.MaxCollectionByteSize {
continue
}
if totalByteSize+tx.ByteSize() > b.config.MaxCollectionByteSize {
break
}
This could act as a "filter", or we could simply remove that over-sized transaction from the mempool
Same comment to the GasLimit
There was a problem hiding this comment.
I think we covered this case by setting the tx max limit to 64K, but you're right there is no harm covering this edge case in case one day we removed tx size limits
There was a problem hiding this comment.
Finally we get to coin-selection problem!
There was a problem hiding this comment.
applied the suggested changes.
…flow/flow-go into ramtin/add-more-limits-to-collections
This PR updates the collection builder logic to consider the maximum gas and byte size limits while building a collection.