blockchain/types: multisig improvement #1001
Conversation
yoomee1313
requested review from
aidan-kwon,
ehnuje,
KimKyungup and
joowon-byun
as code owners
There was a problem hiding this comment.
@yoomee1313 Your suggestion passes isIstanbul value to too many functions. Could you use fork package instead of passing it too much? You can refer our hard fork tests conducted before mainnet launching.
https://github.com/ground-x/klaytn/pull/2356
https://github.com/ground-x/klaytn/pull/2370
yoomee1313
force-pushed
the
incompatiblechange-multisig
branch
from
3a2e168
to
d5faed6
Compare
* multiSig tx is invalid when the number of signatures is larger than the number of registered key * multiSig tx is invalid when it includes invalid signature * multiSig gas cost is charged depend on the number of signature number, not the number of registered key
yoomee1313
force-pushed
the
incompatiblechange-multisig
branch
from
d5faed6
to
e50cb94
Compare
yoomee1313
requested review from
hyochan-brown,
aidan-kwon,
jeongkyun-oh and
ehnuje
|
@aidan-kwon @ehnuje @jeongkyun-oh @hyochan-brown The previous implementation is using valid sigNum when calculating gas cost. so Please take a second look |
| @@ -72,7 +72,7 @@ type AccountKey interface { | |||
| AccountCreationGas(currentBlockNumber uint64) (uint64, error) | |||
|
|
|||
| // SigValidationGas returns gas required to validate a tx with the account. | |||
| SigValidationGas(currentBlockNumber uint64, r RoleType) (uint64, error) | |||
| SigValidationGas(currentBlockNumber uint64, r RoleType, sigNum int) (uint64, error) | |||
There was a problem hiding this comment.
I prefer naming "num+plural" format for something's "number". In this case, numSigs
For me, sigNum sounds like a ID for a signature. e.g., sigNum=100 (it means 100 is signature's number)
Please check below example.
BlockNumber = A (unique) number assigned to a block
Number(Of)Blocks = How many blocks
As we need to merge this change today, please ignore above comment for now.
But please consider it in your later changes.
There was a problem hiding this comment.
Thinking back, it reads like that.. thanks! Revised in new commit 4becba9
| if weightedSum >= a.Threshold { | ||
| return true | ||
| // Validation 2. if isIstanbul is true, check whether invalid signature exists | ||
| if isIstanbul && validSigNum < len(pMap) { |
There was a problem hiding this comment.
Same comment for validSigNum
There was a problem hiding this comment.
Revised in new commit 4becba9
|
@yoomee1313 Please, resolve the conflict |
yoomee1313
dismissed stale reviews from ehnuje, jeongkyun-oh, and aidan-kwon
via
6faa7df
Proposed changes
This PR implements the one of the istanbul compatible change item, multisig improvement.
Istanbul compatible change is not activated since IstanbulCompatibleBlock of cypress and baobab network is nil.
Main idea
len(tx signature) > len(registeredKey), the tx is invalid.dcd39aa commit will be removed after consensus/istanbul: add round to previous hash #997 merged.
Types of changes
Please put an x in the boxes related to your change.
Checklist
Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.
$ make test)Related issues
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...