1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
|
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 <pieter.wuille@gmail.com>) 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 <bitcoin-development@lists.sourceforge.net>;
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: <CAPg+sBiz9TS0-uEMyRs9grecB-sJLd1vVd2DuTo6p+PSGNUUhg@mail.gmail.com>
From: Pieter Wuille <pieter.wuille@gmail.com>
To: Peter Todd <pete@petertodd.org>
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 <bitcoin-development@lists.sourceforge.net>
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: <bitcoin-development.lists.sourceforge.net>
List-Unsubscribe: <https://lists.sourceforge.net/lists/listinfo/bitcoin-development>,
<mailto:bitcoin-development-request@lists.sourceforge.net?subject=unsubscribe>
List-Archive: <http://sourceforge.net/mailarchive/forum.php?forum_name=bitcoin-development>
List-Post: <mailto:bitcoin-development@lists.sourceforge.net>
List-Help: <mailto:bitcoin-development-request@lists.sourceforge.net?subject=help>
List-Subscribe: <https://lists.sourceforge.net/lists/listinfo/bitcoin-development>,
<mailto:bitcoin-development-request@lists.sourceforge.net?subject=subscribe>
X-List-Received-Date: Mon, 15 Dec 2014 18:10:15 -0000
On Mon, Dec 15, 2014 at 1:47 PM, Peter Todd <pete@petertodd.org> 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
|
