Re: [bitcoin-dev] Stumbling into a contentious soft fork activation attempt

[Jeremy <jlrubin@mit.edu>]

[-- Attachment #1: Type: text/plain, Size: 2748 bytes --]

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.

https://github.com/bitcoin/bips/pull/1271
https://github.com/bitcoin/bips/pull/1272

--
@JeremyRubin <https://twitter.com/JeremyRubin>
<https://twitter.com/JeremyRubin>


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
> <https://github.com/bitcoin/bitcoin/pull/21702#discussion_r616853690> 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.
>
>

[-- Attachment #2: Type: text/html, Size: 5093 bytes --]