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
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319
320
321
322
323
324
325
326
327
328
329
330
331
332
333
334
335
336
337
338
339
340
341
342
343
344
345
346
347
348
349
350
351
352
353
354
355
356
357
358
359
360
361
362
363
364
365
366
367
368
369
370
371
372
373
374
375
376
377
378
379
380
381
382
383
384
385
386
387
388
389
390
391
392
393
394
395
396
397
398
399
400
401
402
403
404
405
406
407
408
409
410
411
412
413
414
415
416
417
418
419
420
421
422
423
424
425
426
427
428
429
430
431
432
433
434
435
436
437
438
439
440
441
442
443
444
445
446
447
448
449
450
451
452
453
454
455
456
457
458
459
460
461
462
463
464
465
466
467
468
469
470
471
472
473
474
475
476
477
478
479
480
481
482
483
484
485
486
487
488
489
490
491
492
493
494
495
496
497
498
499
500
501
502
503
504
505
506
507
508
509
510
511
512
513
514
515
516
517
518
519
520
521
522
523
524
525
526
527
528
529
530
531
532
533
534
535
536
537
538
539
540
541
542
543
544
545
546
547
548
549
550
551
552
553
554
555
556
557
558
559
560
561
562
563
564
565
566
567
568
569
570
571
572
573
574
575
576
577
578
579
580
581
582
583
584
585
586
587
588
589
590
591
592
593
594
595
596
597
598
599
600
601
602
603
604
605
606
607
608
609
610
|
Return-Path: <elombrozo@gmail.com>
Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org
[172.17.192.35])
by mail.linuxfoundation.org (Postfix) with ESMTPS id 275E1114E
for <bitcoin-dev@lists.linuxfoundation.org>;
Tue, 15 Sep 2015 16:00:10 +0000 (UTC)
X-Greylist: whitelisted by SQLgrey-1.7.6
Received: from mail-ig0-f182.google.com (mail-ig0-f182.google.com
[209.85.213.182])
by smtp1.linuxfoundation.org (Postfix) with ESMTPS id A3B7D295
for <bitcoin-dev@lists.linuxfoundation.org>;
Tue, 15 Sep 2015 16:00:07 +0000 (UTC)
Received: by igcpb10 with SMTP id pb10so17397694igc.1
for <bitcoin-dev@lists.linuxfoundation.org>;
Tue, 15 Sep 2015 09:00:07 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113;
h=user-agent:in-reply-to:references:mime-version:content-type
:content-transfer-encoding:subject:from:date:to:cc:message-id;
bh=LdqHnixqY+86gUjX6sXFZ/5mPny2SyV1MvkrCss8yiA=;
b=E+i1n3HKQiEAZ5B5SlF/3N1nYZNaCTeVTUQ8qi8q441yBRcfdHzoA4sE1VNjmPeSyC
CHcYEEJVqDcAdeBYRB6/SgvOrTCXsy7NlIju4bh7pSqEh4V80adozA2PEArNeFTiVr92
QgCVIQlmf9EgqNF6OgAXl6U/sqMfaLg8BWiVYA+eM0yBbKYUBbdNWclovi6D1Ef/Thnl
S4iupGtsiCic+BwmfdIzJ0xe2rxQb2vlX9o92Imr9Gp9677TO8p+VvfRi/StLZyjCfIO
48sCGvYS5pbYhq+cmv9KiCbb7yOaLvy3RYqSmdU6QC0z/QK3appNFxEkymGCA4pqJr6b
iaGw==
X-Received: by 10.50.143.4 with SMTP id sa4mr7034843igb.33.1442332806757;
Tue, 15 Sep 2015 09:00:06 -0700 (PDT)
Received: from [172.16.55.99] (74-216-109-222.dedicated.allstream.net.
[74.216.109.222]) by smtp.gmail.com with ESMTPSA id
189sm5978482ioe.40.2015.09.15.09.00.05
(version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);
Tue, 15 Sep 2015 09:00:06 -0700 (PDT)
User-Agent: K-9 Mail for Android
In-Reply-To: <CADm_WcafXgg5xpQjE8uSfp-2y59XRaCUr9w5yNEoVo6GygYzZw@mail.gmail.com>
References: <CADm_WcY8Vy+k+5BaBS+jV6D6tmSXrok8rAxoPxxKOzUhyPWgMg@mail.gmail.com>
<CADJgMzvSUTrPZa0mQvBsQaYkMN3NPXVB_Ay6RJNivq-agUoUww@mail.gmail.com>
<CADm_WcafXgg5xpQjE8uSfp-2y59XRaCUr9w5yNEoVo6GygYzZw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/alternative;
boundary="----9OXJ135928RDPT3K4VXW3C5P4Z6QQQ"
Content-Transfer-Encoding: 8bit
From: Eric Lombrozo <elombrozo@gmail.com>
Date: Tue, 15 Sep 2015 12:00:11 -0400
To: Jeff Garzik <jgarzik@gmail.com>,
Jeff Garzik via bitcoin-dev <bitcoin-dev@lists.linuxfoundation.org>,
Btc Drak <btcdrak@gmail.com>
Message-ID: <6F3F39FB-10A4-469B-8B90-4113A822AB1A@gmail.com>
X-Spam-Status: No, score=-2.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,
DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,HTML_MESSAGE,RCVD_IN_DNSWL_LOW
autolearn=ham version=3.3.1
X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on
smtp1.linux-foundation.org
Cc: Bitcoin development mailing list <bitcoin-dev@lists.linuxfoundation.org>
Subject: Re: [bitcoin-dev] libconsensus and bitcoin development process
X-BeenThere: bitcoin-dev@lists.linuxfoundation.org
X-Mailman-Version: 2.1.12
Precedence: list
List-Id: Bitcoin Development Discussion <bitcoin-dev.lists.linuxfoundation.org>
List-Unsubscribe: <https://lists.linuxfoundation.org/mailman/options/bitcoin-dev>,
<mailto:bitcoin-dev-request@lists.linuxfoundation.org?subject=unsubscribe>
List-Archive: <http://lists.linuxfoundation.org/pipermail/bitcoin-dev/>
List-Post: <mailto:bitcoin-dev@lists.linuxfoundation.org>
List-Help: <mailto:bitcoin-dev-request@lists.linuxfoundation.org?subject=help>
List-Subscribe: <https://lists.linuxfoundation.org/mailman/listinfo/bitcoin-dev>,
<mailto:bitcoin-dev-request@lists.linuxfoundation.org?subject=subscribe>
X-List-Received-Date: Tue, 15 Sep 2015 16:00:10 -0000
------9OXJ135928RDPT3K4VXW3C5P4Z6QQQ
Content-Transfer-Encoding: 8bit
Content-Type: text/plain;
charset=UTF-8
I basically agree with what has been said here.
Refactoring efforts should be well-coordinated. Their short-term impact can be quite disruptive, although if done correctly, longer-term they make it even easier for downstream developers to add and merge changes.
By scheduling move-only changes, others can avoid making PRs immediately prior to or during these changes (which ironically involve considerable disruption to PRs while changing nothing for endusers). Furthermore, it would be useful to document the changes in ways that help other developers rebase properly.
On September 15, 2015 11:26:50 AM EDT, Jeff Garzik via bitcoin-dev <bitcoin-dev@lists.linuxfoundation.org> wrote:
>Drak,
>
>I would say that the refactoring does actually fulfill some conditions
>you
>mention:
>- move-only is almost always clearly separated out
>- the refactoring is not controversial _in minimis_ - meaning, the
>individual pull request is not controversial.
>
>The problem comes with the impact of an unfocused stream of refactors
>to
>key code.
>
>For example, there is much less long term developer impact if
>refactoring
>were _accelerated_, scheduled to be performed in a one-week sprint.
>There
>is a lot of breakage, yes, but after that week the average level of
>downstream patch breakage is significantly lower. A "rip the band-aid
>off
>quickly rather than slowly" approach.
>
>
>
>
>On Tue, Sep 15, 2015 at 5:55 AM, Btc Drak <btcdrak@gmail.com> wrote:
>
>> I also share a lot of Jeff's concerns about refactoring and have
>voiced
>> them several times on IRC and in private to Jorge, Wladamir and Greg.
>I
>> meant to do a write up but never got around to it. Jeff has quite
>> eloquently stated the various problems. I would like to share my
>thoughts
>> on the matter because we really do need to come up with a plan on how
>this
>> issue is dealt with.
>>
>> Obviously, Bitcoin Core is quite tightly coupled at the moment and
>> definitely needs extensive modularisation. Such work will inevitably
>> require lots of bulk code moves and then finer refactoring. However,
>it
>> requires proper planning because there are lots of effects and
>consequences
>> for other people contributing to Core and also downstream projects
>relying
>> on Core:
>>
>> 1. Refactoring often causes other pull requests to diverge and
>require
>> rebasing. Continual refactoring can put PRs in "rebase hell" and puts
>a big
>> stress on contributors (many of whom are part time).
>>
>> 2. Version to version, Bitcoin Core changes significantly in
>structure.
>> 0.9 to 0.10 is unrecognisable. 0.10 to 0.11 is even more so. This
>makes
>> makes it hard to follow release to release and the net result is less
>> people upgrade (especially think of miners trying to keep their patch
>sets
>> working while trying not to disrupt or risk their mining operations).
>>
>> 3. Continual refactoring increases risk: we're human, and mistakes
>will
>> slip through peer review. This is especially concerning with
>consensus
>> critical code and this makes it difficult to merge such refactoring
>often,
>> which of course exacerbates the problem.
>>
>> The net negative consequence is it is harder to contribute to Core,
>harder
>> for the Core maintainers to merge and harder for downstream/dependent
>> projects/implementations to keep up.
>>
>> Suggested Way Forward
>> ---------------------------------
>>
>> With the understanding that refactored code by definition must not
>change
>> behaviour. There are three major kinds of refactoring:
>>
>> 1. code moves (e.g. separating concerns into different files);
>> 2. code style;
>> 3. structural optimisation and consolidation (reducing LOC,
>separating
>> concerns, encapsulation etc).
>>
>> Code moves(1) and CS(2) are easy to peer review and merge quickly.
>The
>> third kind(3) requires deeper analysis to ensure that while the code
>> changed, the behaviour (including any bugs) did not.
>>
>> We must resist all temptation to fix bugs or tack on minor fixes and
>> tweaks during refactoring: pull requests should only be refactoring
>only,
>> with no net change to behaviour. Keeping discipline makes it much
>easier to
>> verify and peer review and this faster to merge.
>>
>> With respect to Code moves and CS, I believe we should have a
>"refactoring
>> fortnight" where we so the bulk of code move-only refactoring plus CS
>where
>> necessary. This is by fat the most disruptive kind of change because
>it
>> widely affects other PRs mergeability. We should aim to get most of
>this
>> done in one go, so that it's not happening in dribs and drabs over
>months
>> and many releases. Once done, it gives everyone a good idea to the
>overall
>> new structure and where one can expect to find things in the future.
>The
>> idea here is to help orientation and not have to continuously hunt
>for
>> where things have moved to.
>>
>> To be clear, I am strongly suggesting code move-only refactoring PRs
>not
>> be mixed with anything else. Same for CS changes. This makes the PRs
>> extremely easy to vet and thus quick to merge.
>>
>> Towards this end, maybe there should be an IRC meeting to agree the
>> initial moves, then someone who has the stomach for it can get on and
>do it
>> - during that time, we do not merge anything else. We need to bite
>the
>> bullet and break the back out of code moves.
>>
>> With regards to CS, I think we do need to get CS right, because a
>> continual dribble of CS changes also makes diffs between releases
>less easy
>> to follow. Much of CS checking can be automated by the continuous
>> integration so authors can get it right easily. It can be just like a
>> Travis check.
>>
>> With respect to the 3rd kind of refactoring, we need to set some
>standards
>> and goals and aim for some kind of consistency. Refactoring needs to
>fulfil
>> certain goals and criterion otherwise contributors will always find a
>> reason to fiddle over and over forever. Obvious targets here can be
>things
>> like proper encapsulation and separation of concerns.
>>
>> Overall, refactoring should be merged quickly, but only on a schedule
>so
>> it doesn't cause major disruption to others.
>>
>> Obviously the third kind of refactoring more complex and time
>consuming
>> and will need to occur over time, but it should happen in defined
>steps. As
>> Jeff said, one week a month, or maybe one month a release. In any
>case,
>> refactoring changes should be quickly accepted or rejected by the
>project
>> maintainer and not left hanging.
>>
>> Finally, refactoring should *always* be uncontroversial because
>> essentially functionality is not changing. If functionality changes
>(e.g.
>> you try to sneak in a big fix or feature tweak "because it's small")
>the PR
>> should be rejected outright. Additionally, if we break down
>refactoring
>> into the three kinds stated above, peer review will be much more
>> straightforward.
>>
>>
>>
>> On Tue, Sep 15, 2015 at 5:10 AM, Jeff Garzik via bitcoin-dev <
>> bitcoin-dev@lists.linuxfoundation.org> wrote:
>>
>>> [collating a private mail and a github issue comment, moving it to a
>>> better forum]
>>>
>>> On libconsensus
>>> ---------------
>>> In general there exists the reasonable goal to move consensus state
>>> and code to a specific, separate lib.
>>>
>>> To someone not closely reviewing the seemingly endless stream of
>>> libconsensus refactoring PRs, the 10,000 foot view is that there is
>a
>>> rather random stream of refactors that proceed in fits and starts
>>> without apparent plan or end other than a one sentence "isolate
>>> consensus state and code" summary.
>>>
>>> I am hoping that
>>> * There is some plan
>>> * We will not see a five year stream of random consensus code
>movement
>>> patches causing lots of downstream developer headaches.
>>>
>>> I read every code change in every pull request that comes into
>>> github/bitcoin/bitcoin with three exceptions:
>>> * consensus code movement changes - too big, too chaotic, too
>>> frequent, too unfocused, laziness guarantees others will inevitably
>>> ACK it without me.
>>> * some non-code changes (docs)
>>> * ignore 80% of the Qt changes
>>>
>>> As with any sort of refactoring, they are easy to prove correct,
>easy
>>> to reason, and therefore quick and easy to ACK and merge.
>>>
>>> Refactors however have a very real negative impact.
>>> bitcoin/bitcoin.git is not only the source tree in the universe.
>>> Software engineers at home, at startups, and at major companies are
>>> maintaining branches of their own.
>>>
>>> It is very very easy to fall into a trap where a project is merging
>>> lots of cosmetic changes and not seeing the downstream ripple
>effects.
>>> Several people complained to me at the conference about all the code
>>> movement changes breaking their own work, causing them to stay on
>>> older versions of bitcoin due to the effort required to rebase to
>each
>>> new release version - and I share those complaints.
>>>
>>> Complex code changes with longer development cycles than simple code
>>> movement patches keep breaking. It is very frustrating, and causes
>>> folks to get trapped between a rock and a hard place:
>>> - Trying to push non-trivial changes upstream is difficult, for
>normal
>>> and reasonable reasons (big important changes need review etc.).
>>> - Maintaining non-trivial changes out of tree is also painful, for
>the
>>> aforementioned reasons.
>>>
>>> Reasonable work languishes in constant-rebase hell, and incentivizes
>>> against keeping up with the latest tree.
>>>
>>>
>>> Aside from the refactor, libconsensus appears to be engineering in
>the
>>> dark. Where is any sort of plan? I have low standards - a photo of
>a
>>> whiteboard or youtube clip will do.
>>>
>>> The general goal is good. But we must not stray into unfocused
>>> engineering for a non-existent future library user.
>>>
>>> The higher priority must be given to having a source code base that
>>> maximizes the collective developers' ability to maintain The Router
>--
>>> the core bitcoin full node P2P engine.
>>>
>>> I recommend time-based bursts of code movement changes. See below;
>>> for example, just submit & merge code movement changes on the first
>>> week of every 2nd month. Code movement changes are easy to create
>>> from scratch once a concrete goal is known. The coding part is
>>> trivial and takes no time.
>>>
>>> As we saw in the Linux kernel - battle lessons hard learned - code
>>> movement and refactors have often unseen negative impact on
>downstream
>>> developers working on more complicated changes that have more
>positive
>>> impact to our developers and users.
>>>
>>>
>>> On Bitcoin development release cycles & process
>>> ------------------------------------------------------------------
>>>
>>> As I've outlined in the past, the Linux kernel maintenance phases
>>> address some of these problems. The merge window into git master
>>> opens for 1 week, a very chaotic week full of merging (and
>rebasing),
>>> and then the merge window closes. Several weeks follow as the "dust
>>> settles" -- testing, bug fixing, moving in parallel OOB with
>>> not-yet-ready development. Release candidates follow, then the
>>> release, then the cycle repeats.
>>>
>>> IMO a merge window approach fixes some of the issues with
>refactoring,
>>> as well as introduces some useful -developer discipline- into the
>>> development process. Bitcoin Core still needs rapid iteration --
>>> another failing of the current project -- and so something of a more
>>> rapid pace is needed:
>>> - 1st week of each month, merge changes. Lots of rebasing during
>this
>>> week.
>>> - remaining days of the month, test, bug fix
>>> - release at end of month
>>>
>>> If changes are not ready for merging, then so be it, they wait until
>>> next month's release. Some releases have major features, some
>>> releases are completely boring and offer little of note. That is
>the
>>> nature of time-based development iteration. It's like dollar cost
>>> averaging, a bit.
>>>
>>>
>>> And frankly, I would like to close all github pull requests that are
>>> not ready to merge That Week. I'm as guilty of this as any, but
>that
>>> stuff just languishes. Excluding a certain category of
>obvious-crap,
>>> pull requests tend to default to a state of either (a) rapid
>merging,
>>> (b) months-long issues/projects, (c) limbo.
>>>
>>> Under a more time-based approach, a better pull request process
>would be
>>> to
>>> * Only open pull requests if it's a bug fix, or the merge window is
>>> open and the change is ready to be merged in the developer's
>opinion.
>>> * Developers CC bitcoin-dev list to discuss Bitcoin Core-bound
>projects
>>> * Developers maintain and publish projects via their own git trees
>>> * Pull requests should be closed if unmerged after 7 days, unless it
>>> is an important bug fix etc.
>>>
>>> The problem with projects like libconsensus is that they can get
>>> unfocused and open ended. Code movement changes in particular are
>>> cheap to generate. It is low developer cost for the developer to
>>> iterate all the way to the end state, see what that looks like, and
>>> see if people like it. That end state is not something you would
>>> merge all in one go. I would likely stash that tree, and then start
>>> again, seek the most optimal and least disruptive set of refactors,
>>> and generate and merge those into bitcoin/bitcoin.git in a
>time-based,
>>> paced manner. Announce the pace ahead of time - "cosmetic stuff
>that
>>> breaks your patches will be merged 1st week of every second month"
>>>
>>> To underscore, the higher priority must be given to having a source
>>> code base and disciplined development process that maximizes the
>>> collective developers' ability to maintain The Router that maintains
>>> most of our network.
>>>
>>> Modularity, refactoring, cleaning up grotty code generates a deep
>>> seated happiness in many engineers. Field experience however shows
>>> refactoring is a never ending process which sometimes gets in the
>way
>>> of More Important Work.
>>> _______________________________________________
>>> bitcoin-dev mailing list
>>> bitcoin-dev@lists.linuxfoundation.org
>>> https://lists.linuxfoundation.org/mailman/listinfo/bitcoin-dev
>>>
>>
>>
>
>
>------------------------------------------------------------------------
>
>_______________________________________________
>bitcoin-dev mailing list
>bitcoin-dev@lists.linuxfoundation.org
>https://lists.linuxfoundation.org/mailman/listinfo/bitcoin-dev
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
------9OXJ135928RDPT3K4VXW3C5P4Z6QQQ
Content-Type: text/html;
charset=utf-8
Content-Transfer-Encoding: 8bit
<html><head></head><body>I basically agree with what has been said here.<br>
<br>
Refactoring efforts should be well-coordinated. Their short-term impact can be quite disruptive, although if done correctly, longer-term they make it even easier for downstream developers to add and merge changes.<br>
<br>
By scheduling move-only changes, others can avoid making PRs immediately prior to or during these changes (which ironically involve considerable disruption to PRs while changing nothing for endusers). Furthermore, it would be useful to document the changes in ways that help other developers rebase properly.<br><br><div class="gmail_quote">On September 15, 2015 11:26:50 AM EDT, Jeff Garzik via bitcoin-dev <bitcoin-dev@lists.linuxfoundation.org> wrote:<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div dir="ltr">Drak,<div><br /></div><div>I would say that the refactoring does actually fulfill some conditions you mention:</div><div>- move-only is almost always clearly separated out</div><div>- the refactoring is not controversial _in minimis_ - meaning, the individual pull request is not controversial.</div><div><br /></div><div>The problem comes with the impact of an unfocused stream of refactors to key code.</div><div><br /></div><div>For example, there is much less long term developer impact if refactoring were _accelerated_, scheduled to be performed in a one-week sprint. There is a lot of breakage, yes, but after that week the average level of downstream patch breakage is significantly lower. A "rip the band-aid off quickly rather than slowly" approach.</div><div><br /></div><div><br /></div><div><br /></div></div><div class="gmail_extra"><br /><div class="gmail_quote">On Tue, Sep 15, 2015 at 5:55 AM, Btc Drak <span dir="ltr"><<a
href="mailto:btcdrak@gmail.com" target="_blank">btcdrak@gmail.com</a>></span> wrote:<br /><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>I also share a lot of Jeff's concerns about refactoring and have voiced them several times on IRC and in private to Jorge, Wladamir and Greg. I meant to do a write up but never got around to it. Jeff has quite eloquently stated the various problems. I would like to share my thoughts on the matter because we really do need to come up with a plan on how this issue is dealt with.<br /></div><div><br /></div><div>Obviously, Bitcoin Core is quite tightly coupled at the moment and definitely needs extensive modularisation. Such work will inevitably require lots of bulk code moves and then finer refactoring. However, it requires proper planning because there are lots of effects and consequences for other people contributing to Core and also downstream projects relying
on
Core: </div><div><br /></div><div>1. Refactoring often causes other pull requests to diverge and require rebasing. Continual refactoring can put PRs in "rebase hell" and puts a big stress on contributors (many of whom are part time).</div><div><br /></div><div>2. Version to version, Bitcoin Core changes significantly in structure. 0.9 to 0.10 is unrecognisable. 0.10 to 0.11 is even more so. This makes makes it hard to follow release to release and the net result is less people upgrade (especially think of miners trying to keep their patch sets working while trying not to disrupt or risk their mining operations).</div><div><br /></div><div>3. Continual refactoring increases risk: we're human, and mistakes will slip through peer review. This is especially concerning with consensus critical code and this makes it difficult to merge such refactoring often, which of course exacerbates the problem. </div><div><br /></div><div>The net negative consequence is it is ha
rder to
contribute to Core, harder for the Core maintainers to merge and harder for downstream/dependent projects/implementations to keep up.</div><div><br /></div><div>Suggested Way Forward<br /></div><div>---------------------------------</div><div><br /></div><div>With the understanding that refactored code by definition must not change behaviour. There are three major kinds of refactoring: <br /></div><div><br /></div><div>1. code moves (e.g. separating concerns into different files);</div><div>2. code style;</div><div>3. structural optimisation and consolidation (reducing LOC, separating concerns, encapsulation etc). </div><div><br /></div><div>Code moves(1) and CS(2) are easy to peer review and merge quickly. The third kind(3) requires deeper analysis to ensure that while the code changed, the behaviour (including any bugs) did not.<br /></div><div><br /></div><div>We must resist all temptation to fix bugs or tack on minor fixes and tweaks during refactoring: pull requests sh
ould
only be refactoring only, with no net change to behaviour. Keeping discipline makes it much easier to verify and peer review and this faster to merge.</div><div><br /></div><div>With respect to Code moves and CS, I believe we should have a "refactoring fortnight" where we so the bulk of code move-only refactoring plus CS where necessary. This is by fat the most disruptive kind of change because it widely affects other PRs mergeability. We should aim to get most of this done in one go, so that it's not happening in dribs and drabs over months and many releases. Once done, it gives everyone a good idea to the overall new structure and where one can expect to find things in the future. The idea here is to help orientation and not have to continuously hunt for where things have moved to.</div><div><br /></div><div>To be clear, I am strongly suggesting code move-only refactoring PRs not be mixed with anything else. Same for CS changes. This makes the PRs extremely
easy to
vet and thus quick to merge.</div><div><br /></div><div><div>Towards this end, maybe there should be an IRC meeting to agree the initial moves, then someone who has the stomach for it can get on and do it - during that time, we do not merge anything else. We need to bite the bullet and break the back out of code moves. <br /></div></div><div><br /></div><div>With regards to CS, I think we do need to get CS right, because a continual dribble of CS changes also makes diffs between releases less easy to follow. Much of CS checking can be automated by the continuous integration so authors can get it right easily. It can be just like a Travis check.</div><div><br /></div><div>With respect to the 3rd kind of refactoring, we need to set some standards and goals and aim for some kind of consistency. Refactoring needs to fulfil certain goals and criterion otherwise contributors will always find a reason to fiddle over and over forever. Obvious targets here can be things like proper
encapsulation and separation of concerns.<br /></div><div><br /></div><div>Overall, refactoring should be merged quickly, but only on a schedule so it doesn't cause major disruption to others. </div><div><br /></div><div>Obviously the third kind of refactoring more complex and time consuming and will need to occur over time, but it should happen in defined steps. As Jeff said, one week a month, or maybe one month a release. In any case, refactoring changes should be quickly accepted or rejected by the project maintainer and not left hanging. </div><div><br /></div><div>Finally, refactoring should *always* be uncontroversial because essentially functionality is not changing. If functionality changes (e.g. you try to sneak in a big fix or feature tweak "because it's small") the PR should be rejected outright. Additionally, if we break down refactoring into the three kinds stated above, peer review will be much more straightforward. </div><div><br /></div><d
iv><br
/></div><div class="gmail_extra"><br /><div class="gmail_quote"><div><div class="h5">On Tue, Sep 15, 2015 at 5:10 AM, Jeff Garzik via bitcoin-dev <span dir="ltr"><<a href="mailto:bitcoin-dev@lists.linuxfoundation.org" target="_blank">bitcoin-dev@lists.linuxfoundation.org</a>></span> wrote:<br /></div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">[collating a private mail and a github issue comment, moving it to a<br />
better forum]<br />
<br />
On libconsensus<br />
---------------<br />
In general there exists the reasonable goal to move consensus state<br />
and code to a specific, separate lib.<br />
<br />
To someone not closely reviewing the seemingly endless stream of<br />
libconsensus refactoring PRs, the 10,000 foot view is that there is a<br />
rather random stream of refactors that proceed in fits and starts<br />
without apparent plan or end other than a one sentence "isolate<br />
consensus state and code" summary.<br />
<br />
I am hoping that<br />
* There is some plan<br />
* We will not see a five year stream of random consensus code movement<br />
patches causing lots of downstream developer headaches.<br />
<br />
I read every code change in every pull request that comes into<br />
github/bitcoin/bitcoin with three exceptions:<br />
* consensus code movement changes - too big, too chaotic, too<br />
frequent, too unfocused, laziness guarantees others will inevitably<br />
ACK it without me.<br />
* some non-code changes (docs)<br />
* ignore 80% of the Qt changes<br />
<br />
As with any sort of refactoring, they are easy to prove correct, easy<br />
to reason, and therefore quick and easy to ACK and merge.<br />
<br />
Refactors however have a very real negative impact.<br />
bitcoin/bitcoin.git is not only the source tree in the universe.<br />
Software engineers at home, at startups, and at major companies are<br />
maintaining branches of their own.<br />
<br />
It is very very easy to fall into a trap where a project is merging<br />
lots of cosmetic changes and not seeing the downstream ripple effects.<br />
Several people complained to me at the conference about all the code<br />
movement changes breaking their own work, causing them to stay on<br />
older versions of bitcoin due to the effort required to rebase to each<br />
new release version - and I share those complaints.<br />
<br />
Complex code changes with longer development cycles than simple code<br />
movement patches keep breaking. It is very frustrating, and causes<br />
folks to get trapped between a rock and a hard place:<br />
- Trying to push non-trivial changes upstream is difficult, for normal<br />
and reasonable reasons (big important changes need review etc.).<br />
- Maintaining non-trivial changes out of tree is also painful, for the<br />
aforementioned reasons.<br />
<br />
Reasonable work languishes in constant-rebase hell, and incentivizes<br />
against keeping up with the latest tree.<br />
<br />
<br />
Aside from the refactor, libconsensus appears to be engineering in the<br />
dark. Where is any sort of plan? I have low standards - a photo of a<br />
whiteboard or youtube clip will do.<br />
<br />
The general goal is good. But we must not stray into unfocused<br />
engineering for a non-existent future library user.<br />
<br />
The higher priority must be given to having a source code base that<br />
maximizes the collective developers' ability to maintain The Router --<br />
the core bitcoin full node P2P engine.<br />
<br />
I recommend time-based bursts of code movement changes. See below;<br />
for example, just submit & merge code movement changes on the first<br />
week of every 2nd month. Code movement changes are easy to create<br />
from scratch once a concrete goal is known. The coding part is<br />
trivial and takes no time.<br />
<br />
As we saw in the Linux kernel - battle lessons hard learned - code<br />
movement and refactors have often unseen negative impact on downstream<br />
developers working on more complicated changes that have more positive<br />
impact to our developers and users.<br />
<br />
<br />
On Bitcoin development release cycles & process<br />
------------------------------------------------------------------<br />
<br />
As I've outlined in the past, the Linux kernel maintenance phases<br />
address some of these problems. The merge window into git master<br />
opens for 1 week, a very chaotic week full of merging (and rebasing),<br />
and then the merge window closes. Several weeks follow as the "dust<br />
settles" -- testing, bug fixing, moving in parallel OOB with<br />
not-yet-ready development. Release candidates follow, then the<br />
release, then the cycle repeats.<br />
<br />
IMO a merge window approach fixes some of the issues with refactoring,<br />
as well as introduces some useful -developer discipline- into the<br />
development process. Bitcoin Core still needs rapid iteration --<br />
another failing of the current project -- and so something of a more<br />
rapid pace is needed:<br />
- 1st week of each month, merge changes. Lots of rebasing during this week.<br />
- remaining days of the month, test, bug fix<br />
- release at end of month<br />
<br />
If changes are not ready for merging, then so be it, they wait until<br />
next month's release. Some releases have major features, some<br />
releases are completely boring and offer little of note. That is the<br />
nature of time-based development iteration. It's like dollar cost<br />
averaging, a bit.<br />
<br />
<br />
And frankly, I would like to close all github pull requests that are<br />
not ready to merge That Week. I'm as guilty of this as any, but that<br />
stuff just languishes. Excluding a certain category of obvious-crap,<br />
pull requests tend to default to a state of either (a) rapid merging,<br />
(b) months-long issues/projects, (c) limbo.<br />
<br />
Under a more time-based approach, a better pull request process would be to<br />
* Only open pull requests if it's a bug fix, or the merge window is<br />
open and the change is ready to be merged in the developer's opinion.<br />
* Developers CC bitcoin-dev list to discuss Bitcoin Core-bound projects<br />
* Developers maintain and publish projects via their own git trees<br />
* Pull requests should be closed if unmerged after 7 days, unless it<br />
is an important bug fix etc.<br />
<br />
The problem with projects like libconsensus is that they can get<br />
unfocused and open ended. Code movement changes in particular are<br />
cheap to generate. It is low developer cost for the developer to<br />
iterate all the way to the end state, see what that looks like, and<br />
see if people like it. That end state is not something you would<br />
merge all in one go. I would likely stash that tree, and then start<br />
again, seek the most optimal and least disruptive set of refactors,<br />
and generate and merge those into bitcoin/bitcoin.git in a time-based,<br />
paced manner. Announce the pace ahead of time - "cosmetic stuff that<br />
breaks your patches will be merged 1st week of every second month"<br />
<br />
To underscore, the higher priority must be given to having a source<br />
code base and disciplined development process that maximizes the<br />
collective developers' ability to maintain The Router that maintains<br />
most of our network.<br />
<br />
Modularity, refactoring, cleaning up grotty code generates a deep<br />
seated happiness in many engineers. Field experience however shows<br />
refactoring is a never ending process which sometimes gets in the way<br />
of More Important Work.<br /></div></div>
_______________________________________________<br />
bitcoin-dev mailing list<br />
<a href="mailto:bitcoin-dev@lists.linuxfoundation.org" target="_blank">bitcoin-dev@lists.linuxfoundation.org</a><br />
<a href="https://lists.linuxfoundation.org/mailman/listinfo/bitcoin-dev" rel="noreferrer" target="_blank">https://lists.linuxfoundation.org/mailman/listinfo/bitcoin-dev</a><br />
</blockquote></div><br /></div></div>
</blockquote></div><br /></div>
<p style="margin-top: 2.5em; margin-bottom: 1em; border-bottom: 1px solid #000"></p><pre class="k9mail"><hr /><br />bitcoin-dev mailing list<br />bitcoin-dev@lists.linuxfoundation.org<br /><a href="https://lists.linuxfoundation.org/mailman/listinfo/bitcoin-dev">https://lists.linuxfoundation.org/mailman/listinfo/bitcoin-dev</a><br /></pre></blockquote></div><br>
-- <br>
Sent from my Android device with K-9 Mail. Please excuse my brevity.</body></html>
------9OXJ135928RDPT3K4VXW3C5P4Z6QQQ--
|