Reimplement Witness/WitnessGroup in terms of TLogPolicy - #1160
Conversation
b87f5a4 to
595d099
Compare
AlCutter
changed the title
d89a2a7 to
fdafe41
Compare
| return WitnessGroup{}, fmt.Errorf("invalid policy: member %q not defined", m) | ||
| } | ||
| } | ||
| wg := NewWitnessGroup(int(g.Threshold), members...) |
There was a problem hiding this comment.
The NewWitnessGroup(n, children...) panics if n < 0 || n > len(children). Do we need any verification here and return error?
There was a problem hiding this comment.
I could add the check, but that would have had to have been an invalid policy which somehow made it through parsing to get to here, in which case panic is probably reasonable?
| w1 := Witness{vkey: vKey1, Key: v1} | ||
| w2 := Witness{vkey: vKey2, Key: v2} |
There was a problem hiding this comment.
Use NewWitness as w1 and w2 are instantiated without setting witName.
| alreadyAdded := false | ||
| for _, existing := range p.Witnesses { | ||
| if existing.Name == witName { | ||
| if existing.Verifier == c.Key && (existing.VKey == c.vkey || c.vkey == "") { |
There was a problem hiding this comment.
Should the comparison be using the key identity (i.e. key name, key hash) instead of the interface pointer?
There was a problem hiding this comment.
Good spot, although actually I think it's probably fine simplify and just check vkey for equality.
| alreadyAdded = true | ||
| break | ||
| } | ||
| witName = fmt.Sprintf("%s-%d", witName, anonGroupNameCounter.Add(1)) |
There was a problem hiding this comment.
By renaming the identical key to -1, one single witness signature would be treated as more than one quorum, potentially bypassing the quorum threshold.
There was a problem hiding this comment.
This is where the key is different (although same name), but actually I think this is also only possible with an invalid policy, so I'll just disallow it.
4163052 to
83d6267
Compare
| if wg.N == 0 || len(wg.Components) == 0 { | ||
| return policy.TLogPolicy{ | ||
| Quorum: "none", | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
Is there a chance that we need to return optional groups when N = 0?
There was a problem hiding this comment.
Good spot, updated.
| } | ||
|
|
||
| func (wg WitnessGroup) toPolicy() (policy.TLogPolicy, error) { | ||
| if wg.N == 0 || len(wg.Components) == 0 { |
There was a problem hiding this comment.
Should we return an error for WitnessGroup{N: 1, Components: nil}?
There was a problem hiding this comment.
Sure, probably overkill given this is all being deprecated, but why not :)
83d6267 to
1b152e5
Compare
|
|
||
| // populatePolicy recursively populates a policy.TLogPolicy from a WitnessGroup. | ||
| // It returns the name of the group and an error if any part of the population fails. | ||
| func populatePolicy(p *policy.TLogPolicy, wg WitnessGroup) (string, error) { |
There was a problem hiding this comment.
The logic here doesn't check the duplicate subgroup name, so it is possible to have the same name appearing more than once in p.Groups. Is that allowed?
sub := NewWitnessGroup(1, w1, w2);
root := NewWitnessGroup(2, sub, sub)There was a problem hiding this comment.
That's not allowed, added a check for that.
This code is all rather temporary though, I'm trying to deprecate and remove all this code which operates in terms of NewWitness and NewWitnessGroup.
1b152e5 to
e49e664
Compare
| } | ||
| alreadyAdded := false | ||
| for _, existing := range p.Witnesses { | ||
| if existing.Name == witName { |
There was a problem hiding this comment.
The current logic doesn't return an error when multiple witnesses use the different name but the same public key. Although this sounds impossible, it's a violation in the tlog-policy specification.
https://github.com/C2SP/C2SP/blob/main/tlog-policy.md#public-key-representation
e49e664 to
1a60fb9
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: df1d8ff | Previous: f1f48c4 | Ratio |
|---|---|---|---|
BenchmarkDedup (github.com/transparency-dev/tessera) |
4432800 ns/op 900912 B/op 18767 allocs/op |
2207022 ns/op 886850 B/op 18650 allocs/op |
2.01 |
BenchmarkDedup (github.com/transparency-dev/tessera) - ns/op |
4432800 ns/op |
2207022 ns/op |
2.01 |
BenchmarkLeafBundle_UnmarshalText (github.com/transparency-dev/tessera/api) |
11550 ns/op 6528 B/op 1 allocs/op |
1351 ns/op 6528 B/op 1 allocs/op |
8.55 |
BenchmarkLeafBundle_UnmarshalText (github.com/transparency-dev/tessera/api) - ns/op |
11550 ns/op |
1351 ns/op |
8.55 |
BenchmarkProofBuilder/InclusionProof/WarmCache (github.com/transparency-dev/tessera/client) |
130331 ns/op 15378 B/op 180 allocs/op |
56548 ns/op 15353 B/op 180 allocs/op |
2.30 |
BenchmarkProofBuilder/InclusionProof/WarmCache (github.com/transparency-dev/tessera/client) - ns/op |
130331 ns/op |
56548 ns/op |
2.30 |
BenchmarkProofBuilder/InclusionProof/ColdCache (github.com/transparency-dev/tessera/client) |
1274427 ns/op 689100 B/op 3555 allocs/op |
554836 ns/op 689106 B/op 3555 allocs/op |
2.30 |
BenchmarkProofBuilder/InclusionProof/ColdCache (github.com/transparency-dev/tessera/client) - ns/op |
1274427 ns/op |
554836 ns/op |
2.30 |
BenchmarkProofBuilder/ConsistencyProof/WarmCache (github.com/transparency-dev/tessera/client) |
125684 ns/op 15453 B/op 180 allocs/op |
56420 ns/op 15411 B/op 180 allocs/op |
2.23 |
BenchmarkProofBuilder/ConsistencyProof/WarmCache (github.com/transparency-dev/tessera/client) - ns/op |
125684 ns/op |
56420 ns/op |
2.23 |
This comment was automatically generated by workflow using github-action-benchmark.
1a60fb9 to
df1d8ff
Compare
2 participants
This PR starts the process of migrating Tessera over to use the policy support implemented in
github.com/transparency-dev/formats/policy.The witness policy c'tors are re-implemented in
TLogPolicy, and helper functions are provided to convert between the two representations.Towards #1152