-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Raft consensus for lotus nodes in a cluster #9294
Raft consensus for lotus nodes in a cluster #9294
Conversation
1dc9115
to
58013e8
Compare
58013e8
to
8f1b1bb
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.
First pass, doesn't feel like there's that much left to do, mostly cleaning up / making stuff more robust.
- Looks like a devnet genesis (
devgen.car
) got committed by accident; Given that it's a little bit we'll need to make sure it's removed from all commits in the PR to not carry it with git history after merging
func (ms *MessageSignerConsensus) RedirectToLeader(ctx context.Context, method string, arg interface{}, ret interface{}) (bool, error) { | ||
ok, err := ms.consensus.RedirectToLeader(method, arg, ret.(*types.SignedMessage)) | ||
if err != nil { | ||
return ok, err | ||
} | ||
return ok, nil | ||
} |
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 we drop this method?
func (ms *MessageSignerConsensus) RedirectToLeader(ctx context.Context, method string, arg interface{}, ret interface{}) (bool, error) { | |
ok, err := ms.consensus.RedirectToLeader(method, arg, ret.(*types.SignedMessage)) | |
if err != nil { | |
return ok, err | |
} | |
return ok, nil | |
} |
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.
No, this is needed to be called in MpoolPushMessage
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.
Looks like there is a commit updating extern/filecoin-ffi
by accident
type NonceMapType map[address.Address]uint64 | ||
type MsgUuidMapType map[uuid.UUID]*types.SignedMessage |
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.
Not needed to land this PR, but if we wanted to be fancy, we could write a generic helper type for maps with typed keys.
ds dtypes.MetadataDS, | ||
consensus *consensus.Consensus) *MessageSignerConsensus { | ||
|
||
ds = namespace.Wrap(ds, datastore.NewKey("/message-signer-consensus/")) |
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 is a different prefix than the one used in non-consensus messagesigner, but I guess that's what we want.
I'm just not 100% sure that we actually need this to be a different prefix.
@@ -667,7 +668,7 @@ func (mp *MessagePool) verifyMsgBeforeAdd(ctx context.Context, m *types.SignedMe | |||
return publish, nil | |||
} | |||
|
|||
func (mp *MessagePool) Push(ctx context.Context, m *types.SignedMessage) (cid.Cid, error) { | |||
func (mp *MessagePool) Push(ctx context.Context, m *types.SignedMessage, publish bool) (cid.Cid, error) { |
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 don't see a similar change for PushUntrusted
. Is that intentional?
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.
Also, could you leave a clear comment on what exactly this bool does?
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 shouldn't need it for PushUntrusted since its not used in syncing the message pool. I think this API should really be folded back into Push itself and add a untrusted param to 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.
Yes I definitely agree :)
Dependency diff if anyone else wants to review: https://gist.github.com/magik6k/ada79bebc4f5ccf3078d72f6513a49e2 |
Related Issues
#9130
Proposed Changes
Adding raft consensus to lotus nodes to maintain consistent state for nonces and messages being published to the lotus node
Additional Info
Checklist
Before you mark the PR ready for review, please make sure that:
<PR type>: <area>: <change being made>
fix: mempool: Introduce a cache for valid signatures
PR type
: fix, feat, INTERFACE BREAKING CHANGE, CONSENSUS BREAKING, build, chore, ci, docs,perf, refactor, revert, style, testarea
: api, chain, state, vm, data transfer, market, mempool, message, block production, multisig, networking, paychan, proving, sealing, wallet, deps