-
Notifications
You must be signed in to change notification settings - Fork 47
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
Convert the type of Quota and QuotaUsed from big.Int to uint64 #654
Conversation
DhunterAO
commented
Jul 4, 2020
- The uint64 type introduce the risk of overflow, so I add checks after add and sub operations, please check whether they are correct or if there are some checks missed.
- The update of storage may cause hard fork of the blockchain, so I only update the type in the memory layer and keep the big.Int in storage layer. Please check whether there is any code in storage influnced.
Convert the type of Quota and QuotaUsed from big.Int to uint64, which can accelerate the system.
…us into uint64Quota
@@ -301,7 +301,7 @@ func (cuckoo *Cuckoo) verifyHeader(chain consensus.ChainReader, header, parent * | |||
return consensus.ErrInvalidNumber | |||
} | |||
|
|||
if header.Quota.Cmp(new(big.Int).Add(parent.Quota, new(big.Int).SetUint64(chain.Config().GetBlockQuota(header.Number)))) != 0 { | |||
if header.Quota < parent.Quota || header.Quota != parent.Quota+chain.Config().GetBlockQuota(header.Number) { |
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.
Whether the second condition contains first one?
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.
The first one consider the overflow of uint64.
For example, parent.Quota = MaxUint64 - 1, blockQuota = 2, header.Quota = 1. This situation satisfies the second condition but dissatisfies the first one.
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.
Can quota reach MaxUint64, we should avoid overflow in quota calculation.
In fact we can calculate when the quota limit will be
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.
Yeah, gasLimit is set for each block own, but the quota is accumulated through the whole blockchain.
For the max quota value in each block is 65536 * 128 (for dolores), which means the limit will be reached at block number 2199023255551
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.
Yes, gas limit from big.int to uint64 because they know the ceiling ethereum/go-ethereum@6f69cdd#diff-afbbe5ab9727e568254cfee647e62876
2199023255551 is huge for block number, should we consider it as ∞ ?
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.
If we can consider it as ∞, I think we can also convert the quota into uint64
header.QuotaUsed = new(big.Int).Set(parent.QuotaUsed) | ||
header.Quota = parent.Quota + chain.Config().GetBlockQuota(header.Number) | ||
if header.Quota < parent.Quota { | ||
panic("quota reaches the upper limit of uint64") |
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 there any possible to be panic here, if there is, big.Int should be more suitable
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.
Due to the consideration of overflow, if the result of addition exceeds the max of uint64, the panic will occur.
If the type is big.Int, it seems to be unlimited.
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.
Yes, the big.Int is used for avoid overflow. If it can be overflow , big.Int should be suitable for quota
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.
If a valiate block satisfy this condition occurs, all the nodes will crash.
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.
we should confirm all the panics in code can't be attacked.
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.
In fact, I just test that the addition overflow will panic automatically in Golang. Not like that in C/C++. So we need to do the check before to prevent the panics.
Codecov Report
@@ Coverage Diff @@
## master #654 +/- ##
==========================================
- Coverage 50.85% 50.84% -0.02%
==========================================
Files 405 407 +2
Lines 51727 51755 +28
==========================================
+ Hits 26308 26316 +8
- Misses 23405 23415 +10
- Partials 2014 2024 +10
|
core/state_transition.go
Outdated
|
||
st.state.SubUpload(st.to(), quota) //64 ~ 1024 bytes | ||
st.state.SubUpload(st.to(), big.NewInt(int64(quota))) //64 ~ 1024 bytes |
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.
big.NewInt(0).SetUint64(value)
The upload
also can be changed to uint64 for it should always below the quota, but it maybe complex, we will think about it in the future
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.
got it
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.
got it
Did you use |
I havn't installed the |
It's great, what we should think about is the overflow of uint64 in this situation |
I have built it and let it keep running for some days and see the metrics chart |
got it |
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.
LGTM