Received-SPF: pass (sog-mx-3.v43.ch3.sourceforge.com: domain of gmail.com
	designates 209.85.213.173 as permitted sender)
	client-ip=209.85.213.173; envelope-from=gmaxwell@gmail.com;
	helo=mail-ig0-f173.google.com; 
MIME-Version: 1.0
In-Reply-To: <20141215124730.GA8321@savin.petertodd.org>
References: <20141215124730.GA8321@savin.petertodd.org>
Date: Mon, 15 Dec 2014 17:38:57 +0000
Message-ID: <CAAS2fgQuLfU=QmK4oyYtEQxiLByLqie9GKDyMs2_2-KZP0qrWA@mail.gmail.com>
From: Gregory Maxwell <gmaxwell@gmail.com>
To: Peter Todd <pete@petertodd.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Cc: Bitcoin Dev <bitcoin-development@lists.sourceforge.net>
Subject: Re: [Bitcoin-development] Recent EvalScript() changes mean
 CHECKLOCKTIMEVERIFY can't be merged
Precedence: list

On Mon, Dec 15, 2014 at 12:47 PM, Peter Todd <pete@petertodd.org> wrote:
[snip]
> Pull-req #4890=C2=B2, specifically commit c7829ea7, changed the

This change was authored more than three months ago and merged more
than two months ago.
[And also, AFAICT, prior to you authoring BIP65]

I didn't participate in that pull-req, though I saw it... it had five
other contributors working on it and I try to have minimal opinions on
code organization and formatting.

But the idea sounded (and still sounds) reasonable to me.  Of course,
anything could still be backed out if it turned out to be ill-advised
(even post 0.10, as I think now we've had months of testing with this
code in place and removing it may be more risky)... but your comments
here are really not timely.
Everyone has limited resources, which is understandable, but the
concerns you are here are ones that didn't involve looking at the code
to raise, and would have been better process wise raised earlier.

> We need to fix this if CHECKLOCKTIMEVERIFY is to be merged.

I don't see why you conclude this. Rather than violating the layering
by re-parsing the transaction as the lower level, just make this data
additional information that is needed available.
Yes, does mean that rebasing an altcoin that made modifications here
will take more effort and understanding of the code than a purely
mechanical change.

> 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 don't agree. The character of this change is fairly narrow. We have
moderately good test coverage here, and there were five participants
on the PR.

> 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

This is all true stuff, but the fact of it doesn't follow that any
particular change was especially risky.

Beyond the general 'things were changed in a way that made rebasing
an-altcoin take more work' do you have a specific concern here?

Other than travling back in time three months and doing something
differently, do you have any suggestions to ameliorate that concern?
E.g. are their additional tests we don't already have that you think
would increase your confidence with respect to specific safety
concerns?

> 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.

There won't be any public users of the library until there can
actually _be_ a library.

PR4890's primary objective was disentangling the script validation
from the node state introduced by the the signature caching changes a
couple years ago, making it possible to build the consensus components
without application specific threading logic... and makes it possible
to have a plain script evaluator call without having to replicate all
of bitcoind's threading, signature cache, etc. logic.  Without a
change like this you can't invoke the script engine without having a
much larger chunk of bitcoind running.

0.10 is a major release, not a maintenance release. It's specifically
in major releases that we make changes which are not purely code
motion and narrow bugfixes (Though many of the changes in 0.10 were
nicely factored into verify pure code motion changes from behavioral
changes). There are many very important, even critical, behavioural
changes in 0.10.  That these changes have their own risks are part of
why they aren't in 0.9.x.