Skip to content
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

Accountability module #505

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

ranchalp
Copy link
Contributor

@ranchalp ranchalp commented Jul 28, 2023

From pkg/accountability/simpleacc/accountability.go:

// NewModule creates a new instance of the (optinal) accountability
// module.
// This module can receive predecisions from an
// ordering module (instead of the ordering module delivering them to the
// application layer directly). It performs two all-to-all broadcasts
// with signatures to ensure accountability. The first broadcast is a
// signed predecision per participant. In the second broadcast, each
// participant broadcasts a certificate containing a strong quorum of
// signed predecisions that they each delivered from the first
// broadcast. Termination occurs once a process receives a strong quorum
// of correctly signed predecisions.
// *Accountability* states that if an adversary (controlling less than a
// strong quorum, but perhaps more or as much as a weak quorum) causes
// a disagreement (two different correct processes delivering different
// decisions) then there is at least a weak quorum of processes for which
// all correct processes eventually receive Proofs-of-Misbehavior (PoMs).
// In the case of this module, a PoM is a pair of signed predecisions for
// different predecisions from the same node.
// The module keeps looking for PoMs with newly received messages
// (signed predecisions or certificates) after termination, until
// it is garbage collected.
//

// Intuition of correctness: a process only terminates if it receives a
// strong quorum of signed predecisions from distinct processes, forming
// a certificate.  Once this process forms a certificate, it shares it
// with the rest of participants. If all correct processes terminate,
// then that means all correct processes will (i) deliver a strong quorum
// of signed predecisions and (ii) broadcast them in a certificate. Thus,
// if two correct processes p_1, p_2 disagree (decide d_1, d_2,
// respectively) then that means they must have each delivered a strong
// quorum of signed predecisions for different predecisions. By quorum
// intersection, this means that at least a weak quorum of processes have
// signed respective predecisions for d_1, d_2 and sent each of them to
// the respective correct process p_1, p_2. Once p_1 receives the
// certificate that p_2 broadcasted, p_1 will then generate a weak quorum
// of PoMs (and vice versa) and broadcast it to the rest of processes.
//
// This module effectively implements a variant of the accountability
// module of Civit et al. at https://ieeexplore.ieee.org/document/9820722/
// Except that it does not implement the optimization using threshold
// signatures (as we have members with associated weight)
//

// The optimistic variant of this module is a parametrizable optimization
// in which certificates are optimistically believed to be correct. This way,
// in the good case a correct process broadcasts a light certificate of O(1) bits
// (instead of O(n) of a certificate)
// and only actually sends the full certificate to nodes from which it receives a light certificate
// for a predecision other than the locally Decided one. The recipient of the certificate can then
// generate and broadcast the PoMs.
//
// ATTENTION: This module is intended to be used once per instance
// (to avoid replay attacks) and reinstantiated in a factory.

Closes #383

TODOs (in later PRs?):

  • Actually use the module in Trantor?
  • Implement tests for the module? (not trivial)

@ranchalp ranchalp force-pushed the accountability-module branch 2 times, most recently from 063d1fc to c7b884b Compare August 2, 2023 12:56
protos/accountabilitypb/accountabilitypb.proto Outdated Show resolved Hide resolved
protos/accountabilitypb/accountabilitypb.proto Outdated Show resolved Hide resolved
pkg/accountability/simpleacc/accountability.go Outdated Show resolved Hide resolved
pkg/accountability/simpleacc/accountability.go Outdated Show resolved Hide resolved
pkg/accountability/simpleacc/accountability.go Outdated Show resolved Hide resolved
Comment on lines 71 to 74
for i, nodeId := range nodeIds {
predecisions.ApplySigVerified(m, mc, params, state, nodeId, errs[i], vsr.certificate[nodeId], false, logger)
}
poms.SendPoMs(m, mc, params, state, logger)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it still be correct to set the flag to true and not call poms.SendPoMs? If yes, the code could be simplified quite a bit by completely removing the flag and always acting as if it was true.
In fact, would we then even still need the UnsentPoMs variable in the state?

Copy link
Contributor Author

@ranchalp ranchalp Aug 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can but in a received certificate for a different decision we will likely detect either no PoMs or at least a weak quorum (e.g. n/3+1) of them. this means n/3+1 events if we remove the flag and sendPoMs for each detected PoM.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case I have the quite strong opinion that we should simplify the code. Finding a PoM should be, in practice, an EXTREMELY rare event, and even in that case, such a performance detail would probably make almost no difference. Simplicity of the code, however, is a very important thing that should take precedence here I think.

Copy link
Contributor

@matejpavlovic matejpavlovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job, I resolved most of the conversations.

One bigger thing does need to be added I think before merging: some form of test that exerces these code paths. With so much added code that has never been run, it is extremely likely that we both missed something that will be obvious once the code is executed.


message SignedPredecision {
option (net.message) = true;
option (mir.struct) = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to specify the mir.struct when we already declare it a message?

moduleParams.Membership = accParams.Membership
moduleParams.RetentionIndex = accParams.RetentionIndex

// Create a new instance of the multisig collector.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy-paste artifact?

Comment on lines 71 to 74
for i, nodeId := range nodeIds {
predecisions.ApplySigVerified(m, mc, params, state, nodeId, errs[i], vsr.certificate[nodeId], false, logger)
}
poms.SendPoMs(m, mc, params, state, logger)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case I have the quite strong opinion that we should simplify the code. Finding a PoM should be, in practice, an EXTREMELY rare event, and even in that case, such a performance detail would probably make almost no difference. Simplicity of the code, however, is a very important thing that should take precedence here I think.

Signed-off-by: Matej Pavlovic <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No fraudproofs generated for equivocations
2 participants