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-1.v29.ch3.sourceforge.com with esmtp (Exim 4.76) (envelope-from ) id 1Y0a62-0001Mt-Vw for bitcoin-development@lists.sourceforge.net; Mon, 15 Dec 2014 18:10:15 +0000 Received-SPF: pass (sog-mx-4.v43.ch3.sourceforge.com: domain of gmail.com designates 209.85.223.174 as permitted sender) client-ip=209.85.223.174; envelope-from=pieter.wuille@gmail.com; helo=mail-ie0-f174.google.com; Received: from mail-ie0-f174.google.com ([209.85.223.174]) by sog-mx-4.v43.ch3.sourceforge.com with esmtps (TLSv1:RC4-SHA:128) (Exim 4.76) id 1Y0a61-0005wz-RL for bitcoin-development@lists.sourceforge.net; Mon, 15 Dec 2014 18:10:14 +0000 Received: by mail-ie0-f174.google.com with SMTP id rl12so11290172iec.19 for ; Mon, 15 Dec 2014 10:10:08 -0800 (PST) MIME-Version: 1.0 X-Received: by 10.42.230.74 with SMTP id jl10mr28894006icb.77.1418667008497; Mon, 15 Dec 2014 10:10:08 -0800 (PST) Received: by 10.50.43.199 with HTTP; Mon, 15 Dec 2014 10:10:08 -0800 (PST) In-Reply-To: <20141215124730.GA8321@savin.petertodd.org> References: <20141215124730.GA8321@savin.petertodd.org> Date: Mon, 15 Dec 2014 19:10:08 +0100 Message-ID: From: Pieter Wuille To: Peter Todd Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-Spam-Score: -1.6 (-) X-Spam-Report: Spam Filtering performed by mx.sourceforge.net. See http://spamassassin.org/tag/ for more details. -1.5 SPF_CHECK_PASS SPF reports sender host as permitted sender for sender-domain 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail provider (pieter.wuille[at]gmail.com) -0.0 SPF_PASS SPF: sender matches SPF record -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature X-Headers-End: 1Y0a61-0005wz-RL 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:10:15 -0000 On Mon, Dec 15, 2014 at 1:47 PM, Peter Todd wrote: > BtcDrak was working on rebasing my CHECKLOCKTIMEVERIFY=B9 patch to master= a few > days ago and found a fairly large design change that makes merging it cur= rently > impossible. Pull-req #4890=B2, specifically commit c7829ea7, changed the > 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. I agree, and I was thinking earlier that some rebasing would be needed for CLTV when the change was made. I think this is a good thing though: #4890 introduced a clear separation between the script evaluation code and what it can access out of its environment (the transaction being verified). As CLTV changes the amount available out of the environment, this indeed requires changing the interface. > We need to fix this if CHECKLOCKTIMEVERIFY is to be merged. Done. See https://github.com/sipa/bitcoin/commit/cltv2 for a rebased version of the BIP65 code on top of 0.10 and master. I haven't ported any tests you may have that are not in the BIP, to avoid doing double work. Those should apply cleanly. There is a less clean version (IMHO) with smaller code changes wrt to the BIP code in my 'cltv' branch too. > 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 fully agree that we shouldn't be taking unnecessary risks when changing consensus code. For example, I closed #5091 (which I would very much have liked as a code improvement) when realizing the risks. That said, I don't believe we are at a point where we can just freeze anything that touches consensus-related, and sometimes refactorings are necessary. In particular, #4890 introduced separation between a very fundamental part of consensus logic (script logic) and an optional optimization for it (caching). If we ever want to get to a separate consensus code tree or repository, possibly with more strict reviews, I think changes like this are inevitable. > 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. I'm sorry to hear that that, and I do understand that many code movements make this harder. If this is a concern shared by many people, we can always decide to roll back some refactorings in the 0.10 branch. On the other hand, we don't even have release candidates yet (which are a pretty important part of the testing and reviewing process), and doing so would delay things further. 0.10 has many very significant improvements which are beneficial to the network too, which I'm sure you're aware of. It's perfectly reasonable that not everyone has the same bandwidth available to keep up with changes, and perhaps that means slowing things down. Again, I don't want to say "this was reviewed before, we can't go back to this" - but did you really need 3 months to realize this change? I also see that elsewhere you're complaining about #5421 of yours which hasn't made it in yet - after less than 2 weeks. Yes, I like the change, and I will review it. Surely you are not arguing it can be merged without decent review? > 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. I have been very much in favor of a libconsensus library, and for several reasons. It's a step towards separating out the consensus-critical parts from optional pieces of the codebase, and it is a step towards avoiding the "reimplementing consensus code is very dangerous! ... but we really don't have a way to allow you to reuse the existing code either" argument. It does not fully accomplish either of those goals, but gradual steps with time to let changes mature in between are nice. > 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. If at this point the consensus code was nicely separated, I would argue to *copy* it (despite all normal engineering practices that argue against code duplication), into a frozen separate directory or even repository, and have all validation related code use the consensus version, while everything else can use a normal tree version, which then can evolve at a normal pace, and undergo cleanups, API changes and feature improvements along with the rest of the code. Unfortunately, it is not. The consensus code is mixed all over the place, and this forces unnecessary strain on all development of the codebase. I hope people agree that getting to a point where this is no longer the case is important. --=20 Pieter