Please see the following bips PRs which are follow ups to the concrete actionables raised by Peter. Thanks for bringing these up, it certainly improves the reviewability of the BIP.




On Mon, Jan 10, 2022 at 7:42 PM Jeremy <jlrubin@mit.edu> wrote:
Hi Peter,

Thank you for your review and feedback.

Apologies for the difficulties in reviewing. The branch linked from the BIP is not the latest, the branch in the PR is what should be considered https://github.com/bitcoin/bitcoin/pull/21702 for review and has more thorough well documented tests and test vectors. The version you reviewed should still be compatible with the current branch as there have not been any spec changes, though.

I'm not sure what best practice is w.r.t. linking to BIPs and implementations given need to rebase and respond to feedback with changes. Appreciate any pointers on how to better solve this. For the time being, I will suggest an edit to point it to the PR, although I recognize this is not ideal. I understand your preference for a commit hash and can do one if it helps. For what it's worth, the taproot BIPs do not link to a reference implementation of Taproot so I'm not sure what best practice is considered these days.

One note that is unfortunate in your review is that there is a discrepancy between the BIP and the implementation (the original reference or the current PR either) in that caching and DoS is not addressed. This was an explicit design goal of CTV and for it not to be mentioned in the BIP (and just the reference) is an oversight on my part to not aid reviewers more explicitly. Compounding this, I accepted a third-party PR to make the BIP more clear as to what is required to implement it that does not have caching (functional correctness), that exposes the issue if implemented by the BIP directly and not by the reference implementation. I have explained this in a review last year to pyskell on the PR that caching is required for non-DoS. I will add a note to the BIP about the importance of caching to avoid DoS as that should make third party implementers aware of the issue.

That said, this is not a mis-considered part of CTV. The reference implementation is specifically designed to not have quadratic hashing and CTV is designed to be friendly to caching to avoid denial of service. It's just a part of the BIP that can be more clear. I will make a PR to more clearly describe how that should happen.