Skip to content

Consensus Review

Luke Champine requested to merge consensus-review into master

whew. This was fun. I was especially nitpicky here so I'm willing to tone it down in places where you think my changes are unnecessary.

Remaining issues:

  • SigHash behaves a bit differently now; instead of manually marshaling and appending the fields, it instead removes all the irrelevant information from the transaction and then marshals the transaction as a whole. I think this should be fine as long as the behavior is well-defined and nothing unexpected winds up being included. Worst case, you marshal a few empty length prefixes.
  • StorageProofOutputID hashes a bool. Bools are currently marshaled as a single byte, either 1 or 0. Again, this is fine as long as the behavior is well defined. That is, people need to know to include a 1 or 0 in the hash, not something like "true" or "false".
  • It took me a while to understand the SurpassThreshold equation. I think I understand it now, but the math is a bit awkward. There might be a better way to do things.
  • There are a few places where code could be organized better. Specifically, I'm thinking of the Target/Big conversion functions, and info.go which is currently a huge mix of exported and unexported functions. It also makes me uneasy having all of the methods of a type defined across 4 separate files. What we have now isn't bad though.
  • The flow of the code was pretty opaque at times. Like, what's the difference between ValidTransaction and ValidTransactionComponents? Who calls which one, and why? Do they overlap? etc.
  • Diffs. Are they really necessary? I think they're helpful going backwards, but how frequently is a diff going to be cached when we're going forward? Seems like something that would only happen if you're switching back and forth between two competing chains. Even still, diffs feel more like an optimization than crucial functionality. I think diff/forking code could be a lot cleaner if we only used diffs going backwards. I might be missing something here though.

Merge request reports