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

types,consensus: Add hashAll helper #129

Merged
merged 1 commit into from
Oct 16, 2023
Merged

types,consensus: Add hashAll helper #129

merged 1 commit into from
Oct 16, 2023

Conversation

lukechampine
Copy link
Member

@lukechampine lukechampine commented Oct 13, 2023

This is a slightly bigger change, so I figured I'd PR it to allow for some discussion instead of just pushing it.

Sia computes a lot of hashes. When I started implementing v2, I took care to make hashing as fast as possible. In particular, I wanted hashing operations to be zero-alloc. That meant accepting some ugliness like hasherPool and eschewing helper functions that might not be inlined.

This PR essentially reverses that decision. Instead of every ID function grabbing a hasher and directly calling EncodeTo on all the stuff we want to encode, we now pass those things to a magic hashAll helper function. The resulting code is definitely more readable; you can see at a glance exactly what's being hashed. The downside is that all of our hashes now allocate, making them a bit slower. How much slower? Anywhere from 1% to 15%, depending on how little data is being hashed. Is that acceptable? Well, when I run our test suite 100x, it takes basically the same amount of time -- definitely nowhere near 15% slower. Regardless -- preventable allocations make me sad. At least ~all of our hashing will be performed inside hashAll now, which means CPU profiles will show us how much time we spend hashing in general...?

Another consequence is that the exact data we're hashing has changed slightly. Basically, instead of omitting signatures, we now hash empty signatures. Why? Because it means instead of writing this:

func (s State) ContractSigHash(fc types.V2FileContract) types.Hash256 {
	return hashAll(
		"sig/filecontract",
		s.v2ReplayPrefix(),
		fc.Filesize,
		fc.FileMerkleRoot,
		fc.ProofHeight,
		fc.ExpirationHeight,
		fc.RenterOutput,
		fc.HostOutput,
		fc.MissedHostValue,
		fc.RenterPublicKey,
		fc.HostPublicKey,
		fc.RevisionNumber,
	)
}

we can write this:

func (s State) ContractSigHash(fc types.V2FileContract) types.Hash256 {
	fc.RenterSignature, fc.HostSignature = types.Signature{}, types.Signature{}
	return hashAll("sig/filecontract", s.v2ReplayPrefix(), fc)
}

Having a helper like hashAll doesn't force us to make this change, but it sure makes it a lot more compelling, doesn't it?

Lemme know if you have strong feelings about this either way. Frankly, I'm pretty confident that this is the right choice; writing a big post about it is probably just my way of coping with the loss of my precious zero-alloc code 😭

@lukechampine lukechampine merged commit c856714 into master Oct 16, 2023
6 checks passed
@lukechampine lukechampine deleted the hashall branch December 4, 2023 22:23
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.

1 participant