New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding gas and byte size limits to collections #91
Changes from all commits
d5c0d3e
e86958c
bd0e821
fcf94c5
60ee0d9
2aa415e
abc1aa3
a82f520
c98fef8
86d6780
aa2a2fa
86cde9c
06f10e7
f7abd01
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -195,13 +195,41 @@ func (b *Builder) BuildOn(parentID flow.Identifier, setter func(*flow.Header) er | |
minRefID := refChainFinalizedID | ||
|
||
var transactions []*flow.TransactionBody | ||
var totalByteSize uint64 | ||
var totalGas uint64 | ||
for _, tx := range b.transactions.All() { | ||
|
||
// if we have reached maximum number of transactions, stop | ||
if uint(len(transactions)) >= b.config.MaxCollectionSize { | ||
break | ||
} | ||
|
||
txByteSize := uint64(tx.ByteSize()) | ||
// ignore transactions with tx byte size bigger that the max amount per collection | ||
// this case shouldn't happen ever since we keep a limit on tx byte size but in case | ||
// we keep this condition | ||
if txByteSize > b.config.MaxCollectionByteSize { | ||
continue | ||
} | ||
|
||
// because the max byte size per tx is way smaller than the max collection byte size, we can stop here and not continue. | ||
// to make it more effective in the future we can continue adding smaller ones | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm concerned by this comment: In the worst case, if the byte size of the first transaction returned by Could we add a check to say if the single transaction's byte size is bigger than the max colleciton byte size, we will
This could act as a "filter", or we could simply remove that over-sized transaction from the mempool Same comment to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Finally we get to coin-selection problem! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. applied the suggested changes. |
||
if totalByteSize+txByteSize > b.config.MaxCollectionByteSize { | ||
break | ||
} | ||
|
||
// ignore transactions with max gas bigger that the max total gas per collection | ||
// this case shouldn't happen ever but in case we keep this condition | ||
if tx.GasLimit > b.config.MaxCollectionTotalGas { | ||
continue | ||
} | ||
|
||
// cause the max gas limit per tx is way smaller than the total max gas per collection, we can stop here and not continue. | ||
// to make it more effective in the future we can continue adding smaller ones | ||
if totalGas+tx.GasLimit > b.config.MaxCollectionTotalGas { | ||
break | ||
} | ||
|
||
// retrieve the main chain header that was used as reference | ||
refHeader, err := b.mainHeaders.ByBlockID(tx.ReferenceBlockID) | ||
if errors.Is(err, storage.ErrNotFound) { | ||
|
@@ -251,6 +279,8 @@ func (b *Builder) BuildOn(parentID flow.Identifier, setter func(*flow.Header) er | |
limiter.transactionIncluded(tx) | ||
|
||
transactions = append(transactions, tx) | ||
totalByteSize += txByteSize | ||
totalGas += tx.GasLimit | ||
} | ||
|
||
// STEP FOUR: we have a set of transactions that are valid to include | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 uint64 | ||
|
||
// MaxCollectionTotalGas is the maximum of total of gas per collection (sum of maxGasLimit over transactions) | ||
MaxCollectionTotalGas uint64 | ||
} | ||
|
||
func DefaultConfig() Config { | ||
|
@@ -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: uint64(1000000), // ~1MB | ||
MaxCollectionTotalGas: uint64(1000000), // 1M | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
} | ||
} | ||
|
||
|
@@ -71,3 +79,15 @@ func WithUnlimitedPayers(payers ...flow.Address) Opt { | |
c.UnlimitedPayers = lookup | ||
} | ||
} | ||
|
||
func WithMaxCollectionByteSize(limit uint64) Opt { | ||
return func(c *Config) { | ||
c.MaxCollectionByteSize = limit | ||
} | ||
} | ||
|
||
func WithMaxCollectionTotalGas(limit uint64) Opt { | ||
return func(c *Config) { | ||
c.MaxCollectionTotalGas = limit | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is fingerprint an ID? I understand that this function, AFAIR just hexprints it, but seems semantically wrong to use
ID()
onfingerprint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point on semantics. the issue is we don't have a fingerprint type, and it still returns an Identifier type