From: Gregory Maxwell <gmaxwell@gmail•com>
To: Peter Todd <pete@petertodd•org>
Cc: Bitcoin Dev <bitcoin-development@lists•sourceforge.net>
Subject: Re: [Bitcoin-development] Recent EvalScript() changes mean CHECKLOCKTIMEVERIFY can't be merged
Date: Mon, 15 Dec 2014 17:38:57 +0000 [thread overview]
Message-ID: <CAAS2fgQuLfU=QmK4oyYtEQxiLByLqie9GKDyMs2_2-KZP0qrWA@mail.gmail.com> (raw)
In-Reply-To: <20141215124730.GA8321@savin.petertodd.org>
On Mon, Dec 15, 2014 at 12:47 PM, Peter Todd <pete@petertodd•org> wrote:
[snip]
> Pull-req #4890², 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 significant
> 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, accept
> 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.
next prev parent reply other threads:[~2014-12-15 17:39 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-12-15 12:47 Peter Todd
2014-12-15 14:57 ` Btc Drak
2014-12-15 15:20 ` Jeff Garzik
2014-12-15 18:42 ` Cory Fields
2014-12-15 19:35 ` Jeff Garzik
2014-12-15 21:19 ` Cory Fields
2014-12-15 21:54 ` Jeff Garzik
2014-12-15 21:57 ` Btc Drak
2014-12-15 17:38 ` Gregory Maxwell [this message]
2014-12-15 17:46 ` Wladimir
2014-12-15 18:10 ` Pieter Wuille
2014-12-15 18:35 ` Cory Fields
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAAS2fgQuLfU=QmK4oyYtEQxiLByLqie9GKDyMs2_2-KZP0qrWA@mail.gmail.com' \
--to=gmaxwell@gmail$(echo .)com \
--cc=bitcoin-development@lists$(echo .)sourceforge.net \
--cc=pete@petertodd$(echo .)org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox