-
Notifications
You must be signed in to change notification settings - Fork 20.4k
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
all: implement simple checkpoint syncing #19543
all: implement simple checkpoint syncing #19543
Conversation
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.
This LGTM. I think it would be good if we can start playing with it in practice and maybe iron out remaining kinks as we find them. Not sure about how the deployment roadmap would look like for this one, can we merge it and have it disabled unless a certain switch (e.g. light.contract-cht
) is used?
cmd/registrar/common.go
Outdated
) | ||
|
||
// newClient creates a client with specified remote URL. | ||
func newClient(ctx *cli.Context) *ethclient.Client { |
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 all of these methods, you call utils.Fatalf
instead of returning an error. It seems that it's generally fine, since they're only called from the exec.go
, however, it's a bit deviating from the norm of returning errors and handling them.
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.
Myeah, I had this debate with @fjl a while back. I wanted to replace all utils.Fatalf
with log.Crit
, but Felix told me that the utils version does some extra cleanup, logging, whatnot that the Crit does not, so from high level cmd
things we kept using the utils.Fatal. Might be good to revisit this decision at some point, simply to unify the API.
@holiman Thanks for your time! In theory we can merge this PR and disable the registrar, so that we will still use the hardcoded checkpoint, it's totally compatible. And we can also enable the registrar for some testnets to see how does it perform. Actually it won't hurt too much if there is a bug in the checkpoint contract since we can redeploy a new one(we need release a new version to change the hardcoded contract address). All old light clients will not receive new checkpoint announcements, but still works. |
1a00256
to
e0d2f8a
Compare
904f121
to
2349d77
Compare
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 new contract looks good and simple.
if (block.number < (_sectionIndex+1)*sectionSize+processConfirms) { | ||
return false; | ||
} | ||
// Filter out "old" announcement |
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.
This logic is fine, I was just thinking about an alternative that is very simple and would even allow fixing a bad checkpoint... instead of rejecting "old" and "stale" checkpoints we could simply require _recentNumber > height
. This would ensure that every signer's intention was to update the current best checkpoint.
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.
How do you mean? What would prevent someone from submiting a stale checkpoint? The submitter can just use a new recentnumber/recenthash. Those aren't signed by the other signers, just passed by the submitter
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 am thinking about deleting the stale checking so that we can enable bad checkpoint fixing.
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.
@holiman you are right, my mistake. It makes no sense then.
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.
@rjl493456442 if we want to enable overwriting existing checkpoints then we should at least add some kind of serial number into the signed data that would prevent re-sending the previous version after the fix. Or we can leave it as it is now, the whole model assumes the reliability of the checkpoint admins anyway and I don't think we will ever set a wrong checkpoint accidentally.
2349d77
to
a2bec5f
Compare
b9107b3
to
2099bc7
Compare
2099bc7
to
9e4835a
Compare
@karalabe Updated. |
afdc797
to
15d322f
Compare
Also take care of the linter https://travis-ci.org/ethereum/go-ethereum/jobs/550214910 |
Fixed. |
694fc03
to
d5fc73b
Compare
cmd, les, node: remove callback mechanism cmd, node: remove callback definition les: simplify the registrar les: expose checkpoint rpc services in the light client les, light: don't store untrusted receipt cmd, contracts, les: discard stale checkpoint cmd, contracts/registrar: loose restriction of registeration cmd, contracts: add replay-protection all: off-chain multi-signature contract params: deploy checkpoint contract for rinkeby cmd/registrar: add raw signing mode for registrar cmd/registrar, contracts/registrar, les: fixed messages
d5fc73b
to
aa2697f
Compare
Please also diff --git a/cmd/checkpoint-admin/exec.go b/cmd/checkpoint-admin/exec.go
index 73c4fa946..3b6e81dac 100644
--- a/cmd/checkpoint-admin/exec.go
+++ b/cmd/checkpoint-admin/exec.go
@@ -25,6 +25,7 @@ import (
"strings"
"time"
+ "github.com/ethereum/go-ethereum/accounts"
"github.com/ethereum/go-ethereum/accounts/abi/bind"
"github.com/ethereum/go-ethereum/cmd/utils"
"github.com/ethereum/go-ethereum/common"
@@ -222,7 +223,7 @@ func sign(ctx *cli.Context) error {
binary.BigEndian.PutUint64(buf, cindex)
p["address"] = address.Hex()
p["message"] = hexutil.Encode(append(buf, chash.Bytes()...))
- if err := clef.Call(&signature, "account_signData", "data/validator", signer, p); err != nil {
+ if err := clef.Call(&signature, "account_signData", accounts.MimetypeDataWithValidator, signer, p); err != nil {
utils.Fatalf("Failed to sign checkpoint, err %v", err)
}
case ctx.GlobalIsSet(keyFileFlag.Name): |
/*/ # * eth: fix sync bloom panic (ethereum#19757). # * eth: fix sync bloom panic. # * eth: delete useless test cases. * mobile: fix mobile interface (ethereum#19180) * mobile: fix mobile interface * mobile, accounts: generate correct java binding * accounts: fix java type binding * mobile: support integer slice * accounts/abi/bind, cmd/abigen: implement java binding tests * les: prefer nil slices over zero-length slices (ethereum#19081) * all: on-chain oracle checkpoint syncing (ethereum#19543) * all: implement simple checkpoint syncing cmd, les, node: remove callback mechanism cmd, node: remove callback definition les: simplify the registrar les: expose checkpoint rpc services in the light client les, light: don't store untrusted receipt cmd, contracts, les: discard stale checkpoint cmd, contracts/registrar: loose restriction of registeration cmd, contracts: add replay-protection all: off-chain multi-signature contract params: deploy checkpoint contract for rinkeby cmd/registrar: add raw signing mode for registrar cmd/registrar, contracts/registrar, les: fixed messages * cmd/registrar, contracts/registrar: fix lints * accounts/abi/bind, les: address comments * cmd, contracts, les, light, params: minor checkpoint sync cleanups * cmd, eth, les, light: move checkpoint config to config file * cmd, eth, les, params: address comments * eth, les, params: address comments * cmd: polish up the checkpoint admin CLI * cmd, contracts, params: deploy new version contract * cmd/checkpoint-admin: add another flag for clef mode signing * cmd, contracts, les: rename and regen checkpoint oracle with abigen * accounts/abi: Fix method overwritten by same name methods. (ethereum#17099) * accounts/abi: Fix method overwritten by same name methods. * accounts/abi: Fix method overwritten by same name methods. * accounts/abi: avoid possible name conflict Co-authored-by: Guillaume Ballet <gballet@gmail.com> * Delete COPYING * Create LICENSE /*/
* eth: fix sync bloom panic (ethereum#19757) * eth: fix sync bloom panic * eth: delete useless test cases * mobile: fix mobile interface (ethereum#19180) * mobile: fix mobile interface * mobile, accounts: generate correct java binding * accounts: fix java type binding * mobile: support integer slice * accounts/abi/bind, cmd/abigen: implement java binding tests * les: prefer nil slices over zero-length slices (ethereum#19081) * all: on-chain oracle checkpoint syncing (ethereum#19543) * all: implement simple checkpoint syncing cmd, les, node: remove callback mechanism cmd, node: remove callback definition les: simplify the registrar les: expose checkpoint rpc services in the light client les, light: don't store untrusted receipt cmd, contracts, les: discard stale checkpoint cmd, contracts/registrar: loose restriction of registeration cmd, contracts: add replay-protection all: off-chain multi-signature contract params: deploy checkpoint contract for rinkeby cmd/registrar: add raw signing mode for registrar cmd/registrar, contracts/registrar, les: fixed messages * cmd/registrar, contracts/registrar: fix lints * accounts/abi/bind, les: address comments * cmd, contracts, les, light, params: minor checkpoint sync cleanups * cmd, eth, les, light: move checkpoint config to config file * cmd, eth, les, params: address comments * eth, les, params: address comments * cmd: polish up the checkpoint admin CLI * cmd, contracts, params: deploy new version contract * cmd/checkpoint-admin: add another flag for clef mode signing * cmd, contracts, les: rename and regen checkpoint oracle with abigen * accounts/abi: Fix method overwritten by same name methods. (ethereum#17099) * accounts/abi: Fix method overwritten by same name methods. * accounts/abi: Fix method overwritten by same name methods. * accounts/abi: avoid possible name conflict Co-authored-by: Guillaume Ballet <gballet@gmail.com> * Delete COPYING * Create LICENSE * Master (#2) (#3) * Create .loc * Rename core/.go/.loc to core/go./loc.js * Delete loc.js * Create .JS * Delete .JS * Update main.yml * Create main.workflow * Update .travis.yml * Update .travis.yml * Delete .travis.yml
* eth: fix sync bloom panic (ethereum#19757) * eth: fix sync bloom panic * eth: delete useless test cases * mobile: fix mobile interface (ethereum#19180) * mobile: fix mobile interface * mobile, accounts: generate correct java binding * accounts: fix java type binding * mobile: support integer slice * accounts/abi/bind, cmd/abigen: implement java binding tests * les: prefer nil slices over zero-length slices (ethereum#19081) * all: on-chain oracle checkpoint syncing (ethereum#19543) * all: implement simple checkpoint syncing cmd, les, node: remove callback mechanism cmd, node: remove callback definition les: simplify the registrar les: expose checkpoint rpc services in the light client les, light: don't store untrusted receipt cmd, contracts, les: discard stale checkpoint cmd, contracts/registrar: loose restriction of registeration cmd, contracts: add replay-protection all: off-chain multi-signature contract params: deploy checkpoint contract for rinkeby cmd/registrar: add raw signing mode for registrar cmd/registrar, contracts/registrar, les: fixed messages * cmd/registrar, contracts/registrar: fix lints * accounts/abi/bind, les: address comments * cmd, contracts, les, light, params: minor checkpoint sync cleanups * cmd, eth, les, light: move checkpoint config to config file * cmd, eth, les, params: address comments * eth, les, params: address comments * cmd: polish up the checkpoint admin CLI * cmd, contracts, params: deploy new version contract * cmd/checkpoint-admin: add another flag for clef mode signing * cmd, contracts, les: rename and regen checkpoint oracle with abigen * accounts/abi: Fix method overwritten by same name methods. (ethereum#17099) * accounts/abi: Fix method overwritten by same name methods. * accounts/abi: Fix method overwritten by same name methods. * accounts/abi: avoid possible name conflict Co-authored-by: Guillaume Ballet <gballet@gmail.com> * Delete COPYING * Create LICENSE * Master (#2) (#3) * Create .loc * Rename core/.go/.loc to core/go./loc.js * Delete loc.js * Create .JS * Delete .JS * Update main.yml * Create main.workflow * Update .travis.yml * Update .travis.yml * Delete .travis.yml
* eth: fix sync bloom panic (ethereum#19757) * eth: fix sync bloom panic * eth: delete useless test cases * mobile: fix mobile interface (ethereum#19180) * mobile: fix mobile interface * mobile, accounts: generate correct java binding * accounts: fix java type binding * mobile: support integer slice * accounts/abi/bind, cmd/abigen: implement java binding tests * les: prefer nil slices over zero-length slices (ethereum#19081) * all: on-chain oracle checkpoint syncing (ethereum#19543) * all: implement simple checkpoint syncing cmd, les, node: remove callback mechanism cmd, node: remove callback definition les: simplify the registrar les: expose checkpoint rpc services in the light client les, light: don't store untrusted receipt cmd, contracts, les: discard stale checkpoint cmd, contracts/registrar: loose restriction of registeration cmd, contracts: add replay-protection all: off-chain multi-signature contract params: deploy checkpoint contract for rinkeby cmd/registrar: add raw signing mode for registrar cmd/registrar, contracts/registrar, les: fixed messages * cmd/registrar, contracts/registrar: fix lints * accounts/abi/bind, les: address comments * cmd, contracts, les, light, params: minor checkpoint sync cleanups * cmd, eth, les, light: move checkpoint config to config file * cmd, eth, les, params: address comments * eth, les, params: address comments * cmd: polish up the checkpoint admin CLI * cmd, contracts, params: deploy new version contract * cmd/checkpoint-admin: add another flag for clef mode signing * cmd, contracts, les: rename and regen checkpoint oracle with abigen * accounts/abi: Fix method overwritten by same name methods. (ethereum#17099) * accounts/abi: Fix method overwritten by same name methods. * accounts/abi: Fix method overwritten by same name methods. * accounts/abi: avoid possible name conflict Co-authored-by: Guillaume Ballet <gballet@gmail.com> * Delete COPYING * Create LICENSE * Master (#2) (#3) * Create .loc * Rename core/.go/.loc to core/go./loc.js * Delete loc.js * Create .JS * Delete .JS * Update main.yml * Create main.workflow * Update .travis.yml * Update .travis.yml * Delete .travis.yml * Delete SECURITY.md
This PR is the successor of #17578
New version contract is