Return-Path: Received: from smtp1.osuosl.org (smtp1.osuosl.org [IPv6:2605:bc80:3010::138]) by lists.linuxfoundation.org (Postfix) with ESMTP id 4C984C001E for ; Tue, 11 Jan 2022 04:38:59 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp1.osuosl.org (Postfix) with ESMTP id 2C96C83459 for ; Tue, 11 Jan 2022 04:38:59 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org X-Spam-Flag: NO X-Spam-Score: -4.197 X-Spam-Level: X-Spam-Status: No, score=-4.197 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_MED=-2.3, RCVD_IN_MSPIKE_H3=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001] autolearn=ham autolearn_force=no Received: from smtp1.osuosl.org ([127.0.0.1]) by localhost (smtp1.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id pz2ndCsO64zO for ; Tue, 11 Jan 2022 04:38:57 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.8.0 Received: from outgoing.mit.edu (outgoing-auth-1.mit.edu [18.9.28.11]) by smtp1.osuosl.org (Postfix) with ESMTPS id 21B4682D04 for ; Tue, 11 Jan 2022 04:38:56 +0000 (UTC) Received: from mail-lf1-f52.google.com (mail-lf1-f52.google.com [209.85.167.52]) (authenticated bits=0) (User authenticated as jlrubin@ATHENA.MIT.EDU) by outgoing.mit.edu (8.14.7/8.12.4) with ESMTP id 20B4csKS004467 (version=TLSv1/SSLv3 cipher=AES128-GCM-SHA256 bits=128 verify=NOT) for ; Mon, 10 Jan 2022 23:38:55 -0500 Received: by mail-lf1-f52.google.com with SMTP id i31so51851530lfv.10 for ; Mon, 10 Jan 2022 20:38:55 -0800 (PST) X-Gm-Message-State: AOAM533wq3CFxT+KuxAlpygUXnsULTu5+yYZYTUjbO7Tov+WlA+Vh4iB iIsMwP1tTL4BHXMYRhjQcdF/4Y3Y1bKANual/f4= X-Google-Smtp-Source: ABdhPJzsCFF1SxACUkj2ynEkKbItqOhASGhD6wCpP1k22Xc+mETon2bE4ddfeFTJNlnaQEK8pNWVWv1JfBEsVwyLa0g= X-Received: by 2002:a05:651c:160a:: with SMTP id f10mr1691013ljq.212.1641875933697; Mon, 10 Jan 2022 20:38:53 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Jeremy Date: Mon, 10 Jan 2022 20:38:42 -0800 X-Gmail-Original-Message-ID: Message-ID: To: Jeremy Content-Type: multipart/alternative; boundary="000000000000cb1c7d05d54707a9" Cc: Bitcoin Protocol Discussion Subject: Re: [bitcoin-dev] Stumbling into a contentious soft fork activation attempt X-BeenThere: bitcoin-dev@lists.linuxfoundation.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: Bitcoin Protocol Discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 11 Jan 2022 04:38:59 -0000 --000000000000cb1c7d05d54707a9 Content-Type: text/plain; charset="UTF-8" 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 On Mon, Jan 10, 2022 at 7:42 PM Jeremy 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. > > --000000000000cb1c7d05d54707a9 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Please s= ee the following bips PRs which are follow ups to the concrete actionables = raised by Peter. Thanks for bringing these up, it certainly improves the re= viewability of the BIP.

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



On = Mon, Jan 10, 2022 at 7:42 PM Jeremy <= jlrubin@mit.edu> wrote:
<= div style=3D"font-family:arial,helvetica,sans-serif;font-size:small;color:r= gb(0,0,0)">Hi Peter,

Thank you for y= our review and feedback.

Apologies f= or the difficulties in reviewing. The branch linked from the BIP is not the= latest, the branch in the PR is what should be considered=C2=A0https://gi= thub.com/bitcoin/bitcoin/pull/21702 for review and has more thorough we= ll documented tests and test vectors. The version you reviewed should still= be compatible with the current branch as there have not been any spec chan= ges, though.

I'm not sure what best practice= is w.r.t. linking to BIPs and implementations given need to rebase and res= pond to feedback with changes. Appreciate any pointers on how to better sol= ve 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 impl= ementation of Taproot so I'm not sure what best practice is considered = these days.

One note that is unfortun= ate in your review is that there is a discrepancy=C2=A0between the BIP and = the implementation (the original reference or the current PR either) in tha= t 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 o= versight 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 requi= red to implement it that does not have caching (functional correctness), th= at exposes the issue if implemented by the BIP directly and not by the refe= rence implementation. I have explained this in a rev= iew 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=C2=A0= part of CTV. The reference implementation is specifically designed to not h= ave quadratic hashing and CTV is designed to be friendly to caching to avoi= d 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.

--000000000000cb1c7d05d54707a9--