From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from sog-mx-4.v43.ch3.sourceforge.com ([172.29.43.194] helo=mx.sourceforge.net) by sfs-ml-2.v29.ch3.sourceforge.com with esmtp (Exim 4.76) (envelope-from ) id 1Y0aqr-0000oW-84 for bitcoin-development@lists.sourceforge.net; Mon, 15 Dec 2014 18:58:37 +0000 X-ACL-Warn: Received: from mail-oi0-f44.google.com ([209.85.218.44]) by sog-mx-4.v43.ch3.sourceforge.com with esmtps (TLSv1:RC4-SHA:128) (Exim 4.76) id 1Y0aqp-0007Po-S8 for bitcoin-development@lists.sourceforge.net; Mon, 15 Dec 2014 18:58:37 +0000 Received: by mail-oi0-f44.google.com with SMTP id e131so8500331oig.17 for ; Mon, 15 Dec 2014 10:58:30 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=iJLMwdhhbT5uRMo46MSf+a/W+YDbRJITvLOgme/K3SU=; b=MbIsgMUtSpVPiepZk4jSIeoaQ44M/nse2Bq9pzzOnen5sHe3jpMZiqXnxcIaVKd3n2 DyqpZas3JZEbBHMVVqMo4pOh20UmwWRXW0orjfpGz2B2RSSX6xZP1EXGrQkBnvyOnvpX zyURDiQCQAUcPlxS6luWzlpztaSTClusWAVaVIHF0zv81wtDzQpEZdfxm4vEmoHJQe/L 7dRo7TCOzMHbHyTlcD8Xzyjbee9hM/cxCbLIlOkzb/V+QavF1MbM6GNu954UilSyKsY1 tZcPiFkdj68RELqUIXaGF7EkTG9HE/AJwhp2TIxUwp8/7/jPewgBYeR9qUvCyZiTnsSa FAeg== X-Gm-Message-State: ALoCoQm8jA6qPkv5xNAmw/1ebVrkFhXJLftChBXBlEKe/ZvQlFBgSsGcE65ycQ3Dtf/L9OY2HrUu MIME-Version: 1.0 X-Received: by 10.182.246.40 with SMTP id xt8mr20048927obc.59.1418668511690; Mon, 15 Dec 2014 10:35:11 -0800 (PST) Received: by 10.182.13.38 with HTTP; Mon, 15 Dec 2014 10:35:11 -0800 (PST) In-Reply-To: <20141215124730.GA8321@savin.petertodd.org> References: <20141215124730.GA8321@savin.petertodd.org> Date: Mon, 15 Dec 2014 13:35:11 -0500 Message-ID: From: Cory Fields To: Peter Todd Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Spam-Score: 1.0 (+) X-Spam-Report: Spam Filtering performed by mx.sourceforge.net. See http://spamassassin.org/tag/ for more details. 1.0 UC_GIBBERISH_OBFU Multiple instances of "word VERYLONGGIBBERISH word" X-Headers-End: 1Y0aqp-0007Po-S8 Cc: Bitcoin Dev Subject: Re: [Bitcoin-development] Recent EvalScript() changes mean CHECKLOCKTIMEVERIFY can't be merged X-BeenThere: bitcoin-development@lists.sourceforge.net X-Mailman-Version: 2.1.9 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 15 Dec 2014 18:58:37 -0000 On Mon, Dec 15, 2014 at 7:47 AM, Peter Todd wrote: > BtcDrak was working on rebasing my CHECKLOCKTIMEVERIFY=C2=B9 patch to mas= ter a few > days ago and found a fairly large design change that makes merging it cur= rently > impossible. Pull-req #4890=C2=B2, specifically commit c7829ea7, changed t= he > EvalScript() function to take an abstract SignatureChecker object, removi= ng the > txTo and nIn arguments that used to contain the transaction the script wa= s in > and the txin # respectively. CHECKLOCKTIMEVERIFY needs txTo to obtain the > nLockTime field of the transaction, and it needs nIn to obtain the nSeque= nce of > the txin. > > We need to fix this if CHECKLOCKTIMEVERIFY is to be merged. > > Secondly, that this change was made, and the manner in which is was made,= is I > think indicative of a development process that has been taking significan= t > risks with regard to refactoring the consensus critical codebase. I know = I > personally have had a hard time keeping up with the very large volume of = code > being moved and changed for the v0.10 release, and I know BtcDrak - who i= s > keeping Viacoin up to date with v0.10 - has also had a hard time giving t= he > changes reasonable review. The #4890 pull-req in question had no ACKs at = all, > and only two untested utACKS, which I find worrying for something that ma= de > significant consensus critical code changes. > > While it would be nice to have a library encapsulating the consensus code= , this > shouldn't come at the cost of safety, especially when the actual users of= that > library or their needs is still uncertain. This is after all a multi-bill= ion > project where a simple fork will cost miners alone tens of thousands of d= ollars > an hour; easily much more if it results in users being defrauded. That's = also > not taking into account the significant negative PR impact and loss of tr= ust. I > personally would recommend *not* upgrading to v0.10 due to these issues. > > A much safer approach would be to keep the code changes required for a > consensus library to only simple movements of code for this release, acce= pt > that the interface to that library won't be ideal, and wait until we have > feedback from multiple opensource projects with publicly evaluatable code= on > where to go next with the API. > > 1) https://github.com/bitcoin/bips/blob/master/bip-0065.mediawiki > 2) https://github.com/bitcoin/bitcoin/pull/4890 > > -- > 'peter'[:-1]@petertodd.org > 00000000000000001b18a596ecadd07c0e49620fb71b16f9e41131df9fc52fa6 It would appear as though you're trying to drum up controversy here, but the argument is quite a stretch, and contrary to some other arguments you're making in parallel. There seem to be three themes in your above complaint, so I'd like to address them individually. 1. Pr #4890 specifically. The argument seems to be that this was not properly reviewed/tested, and that it is an unnecessary risk to the consensus codebase. Looking at the PR at github, while I certainly don't agree with those conclusions, I suppose I can understand where they're coming from. There's plenty of context missing, as well as sidebar discussions on IRC and other PRs. To an outside observer, these changes may look under-tested and unnecessary. The context that's missing is the flurry of work that was going on in parallel to modularize this (and surrounding code). #4890 was one of the first pieces necessary for that, so some of the discussion about it was happening in dependent pull requests. You can point to a lack ACKs in one place for that PR, but that doesn't mean that the changes weren't tested/reviewed/necessary. You could also argue that ACKs should've been mirrored on the PR in question for posterity, which would be a perfectly reasonable argument that I would agree with. 2. These changes conflict with a rebased version of your CHECKLOCKTIMEVERIFY changes. OK? You have a tree that's a few months old, and you find that you have conflicts when rebasing to master. It happens to all of us. Do as the rest of us do and update your changes to fit. If you missed the review of #4890 and think it should be reverted, then call for a revert. But please give a concrete reason other than "I've picked this commit series for a crusade because it gave me merge conflicts". What is the conspiracy here? There's a signature cache that is implementation-specific, and in a parallel universe, you might be arguing that we should rip it out because it adds unnecessary complexity to the consensus code. The PR provides a path around that complexity. For some reason, your reaction is to cry foul months later because you missed reviewing it at the time, rather than cheering for the reduced complexity. 3. You seem to think that 1. and 2. seem to point to a systemic failure of the review process because modularization "shouldn't come at the cost of safety". I agree that it shouldn't come at the cost of safety, but I see no failure here. There has been a HUGE effort to modularize the code with a combination of pure-code-movement and small interface reworks. Please take a moment to grep the git logs for "MOVEONLY" in the 0.10 branch. You'll notice that script verification is now 100% free of bitcoind state, threading, and third-party libraries (other than openssl for now). That constitutes a massive reduction in code complexity, future review overhead, etc. I'll point out here that those were my reasons for my contributions to the libbitcoinconsensus effort. I have no interest in altcoins or sidechains. Those milestones were thanks to an effort which included #4890. If you have issues with these changes and/or how they were made, please call out individual failures and proposed solutions _in context_. That they conflict with CHECKLOCKTIMEVERIFY may be a valid concern, and it may be worth evaluating how separate coding efforts may more effectively parallelized. Without pointers to specific failures or solutions, I'm not sure what you were trying to communicate here, other than maybe stirring the social networks with: "I personally would recommend *not* upgrading to v0.10 due to these issues." That's fine I suppose, but it does nothing to solve whatever issue you're trying to call out here. Cory