Move a constant down the call stack.

[pphaneuf]

Part of #2378 and #2245.

Checklist

• I have updated the CHANGELOG.
• I have updated documentation accordingly (including the feature implementation matrix).

[Martin2112]

I don't think that this support should have been removed. It has nothing to do with the signature stuff you were working on.

[pphaneuf]

It's already all gone from the API, and is currently hardcoded, just at a higher level:

trillian/log/sequencer_manager.go

Line 57 in 41c0759

sequencer := NewSequencer(rfc6962.DefaultHasher, info.TimeSource, s.registry.LogStorage, s.registry.MetricFactory, s.registry.QuotaManager)

[Martin2112]

I know. I didn't get to review that PR so didn't get a chance to raise an issue about it.

[pphaneuf]

This was really more of a continuation of the removal of the map code (all the other hashing schemes still there were map-specific, so had been removed already), and removing unused RPCs and fields in RPC request/response protobufs (once there was just one hashing scheme, nothing set this field anymore).

[pphaneuf]

Are we good with this? This is a very mechanical change, it should be easy to validate that it has no effect other than reducing complexity.

[pav-kv]

I have a mixed feeling about this PR.

• I like the idea of reducing the usage of LogHasher interface, as in Go it's generally better to define interfaces where they are used. For example, here I could imagine a sequencer's own interface with only those methods that it uses: HashChildren and EmptyRoot.
• As a further improvement, I find the EmptyRoot method unnecessary, esp. storing its result in the storage, esp. now that it's not signed. It can be easily handled by upper layers (CTFE), as it's just a constant specific to RFC.
• So, ideally, the sequencer would only have a HashChildren callback as input, similarly to compact.Range.

Hard-coding the RFC hasher seems a bit too much to me, I would keep it slightly more abstract. It's sometimes convenient for testing to provide a custom "human-readable" hasher, e.g. we have something like this internally:

// nodeHash returns a test "hash" for the given tree node. It looks like
// treeID:[begin,end) where [begin,end) is the leaves range under this node.
func nodeHash(treeID int64, id compact.NodeID) []byte {
	begin, end := id.Index<<id.Level, (id.Index+1)<<id.Level
	return []byte(fmt.Sprintf("%d:[%d,%d)", treeID, begin, end))
}

// glueChildren is a test "hash" that simply concatenates parts of two child
// hashes in such a way that the result corresponds to the string that nodeHash
// function would generate for their parent node.
func glueChildren(left, right []byte) []byte {
	l := bytes.IndexByte(left, ',')
	r := bytes.IndexByte(right, ',')
	return append(append([]byte{}, left[:l]...), right[r:]...)
}

We don't use it here, but I can easily see how it could be used (instead / in addition to "golden" tests with long unreadable hashes).

[pphaneuf]

This commit message was initially poorly formulated, and has been updated.

This does not reduce the usage of the LogHasher interface (even though I also agree that it would be a good idea), it only moves a constant down one level. The RFC hasher was already hard-coded, and in practice, there will not be any other hashing scheme until we leave SHA256, which we should apply the YAGNI principle to, and only do it when we do it.

The EmptyRoot method is rather baked into the Trillian principle of a "hashing algorithm", by which we really mean the combination of a hashing algorithm such as SHA256, along with a set of higher-level algorithms around how it is used, which in the case of Trillian as it currently stands, involves what the root hash of an empty tree is supposed to be like, as well as how child hashes are combined and the exact prefix scheme used to protect against 2nd pre-image attacks.

I agree that the particular aspect of dealing with empty trees should probably not be handled by Trillian, but this is a much bigger change, which I am not currently proposing, and should probably not happen with Trillian in its current form (of an out-of-process RPC server), but incorporated into the "next generation" (which is more likely to be packaged as an in-process library).

But all of this is far beyond the scope of this PR, which merely applies constant folding in order to reduce the complexity and the cognitive load for people reading this code. I would gladly review PRs to move towards what you are proposing, though!

[NatalieDoduc]

Good point, Pavel, on potential improvements and the custom hasher for testing.