sipa / bitcoin

Bitcoin integration/staging tree
http://www.bitcoin.org
MIT License
88 stars 21 forks source link

Refactor Taproot execution to be more straightforward #116

Closed JeremyRubin closed 4 years ago

JeremyRubin commented 5 years ago

Hi @sipa!

I've been reviewing the taproot code -- I believe this version to be equivalent to the one proposed.

However, I think that it is much easier to read and review, despite being slightly longer.

Non exhaustive, but here's a list of changes made:

1) Break out 'magic numbers' to constants where possible 2) completely separate witness version execution from one another, duplicating code where necessary 3) Make variables const where possible, move initialization/creation closer to use 4) Refactor non-obvious logic for valid control length checks 5) Simplify the merkle tree building code

It also seems to me to be easier to review to be consistent with the prior code, given that there are minimal changes to the existing segwit v0 flow.

I would also suggest interface-wise another EvalScript method with no ExecData argument and set it to an EvalScript local execdata given that SegWit v0 passes an empty argument, but that's larger churn that might not be worth it.

JeremyRubin commented 4 years ago

It seems like @sipa has addressed most, if not all, of the feedback here, so closing.