<div dir="ltr">This is a pretty good example about refactoring discipline as well as premature/over optimisation.<div><br></div><div>We all want to see more modular code, but the first steps should just be to relocate blocks of code so everything is more logically organised in smaller files (especially for consensus critical code). Refactoring should come in a second wave preferably after a stable release. Refactoring should be in the pure sense, optimising code with absolutely no change in behaviour.</div><div><br></div><div>When it comes to actual API changes, I think we need to be a lot more careful and should be considered feature requests and get a lot more scrutiny as we are essentially breaking backwards compatibility. #4890 was pretty much merged with no discussion or thought yet other really simple and uncontroversial PRs remain unmerged for months. A key question in the case of EvalScript() would have been, "why are we passing txTo and nIn here, and are there any future use cases that might require them? Why should this be removed from the API and the entire method signature changed?". BC breaks always need strong justification.<div><br></div><div>So I've expressed my concern a few times about the speed and frequency of refactoring and also the way it's being done. I am not alone, as others not directly connected with the Bitcoin Core project have also expressed concerns about the number of refactorings "for the sake of refactoring", especially of consensus critical code. Careful as we may be, we know from history that small edge case bugs can creep in very easily and cause a lot of unforeseen problems.<br></div><div><div><br></div><div>BtcDrak<br></div></div></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Dec 15, 2014 at 12:47 PM, Peter Todd <span dir="ltr"><<a href="mailto:pete@petertodd.org" target="_blank">pete@petertodd.org</a>></span> wrote:<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">BtcDrak was working on rebasing my CHECKLOCKTIMEVERIFY¹ patch to master a few<br>
days ago and found a fairly large design change that makes merging it currently<br>
impossible. Pull-req #4890², specifically commit c7829ea7, changed the<br>
EvalScript() function to take an abstract SignatureChecker object, removing the<br>
txTo and nIn arguments that used to contain the transaction the script was in<br>
and the txin # respectively. CHECKLOCKTIMEVERIFY needs txTo to obtain the<br>
nLockTime field of the transaction, and it needs nIn to obtain the nSequence of<br>
the txin.<br>
<br>
We need to fix this if CHECKLOCKTIMEVERIFY is to be merged.<br>
<br>
Secondly, that this change was made, and the manner in which is was made, is I<br>
think indicative of a development process that has been taking significant<br>
risks with regard to refactoring the consensus critical codebase. I know I<br>
personally have had a hard time keeping up with the very large volume of code<br>
being moved and changed for the v0.10 release, and I know BtcDrak - who is<br>
keeping Viacoin up to date with v0.10 - has also had a hard time giving the<br>
changes reasonable review. The #4890 pull-req in question had no ACKs at all,<br>
and only two untested utACKS, which I find worrying for something that made<br>
significant consensus critical code changes.<br>
<br>
While it would be nice to have a library encapsulating the consensus code, this<br>
shouldn't come at the cost of safety, especially when the actual users of that<br>
library or their needs is still uncertain. This is after all a multi-billion<br>
project where a simple fork will cost miners alone tens of thousands of dollars<br>
an hour; easily much more if it results in users being defrauded. That's also<br>
not taking into account the significant negative PR impact and loss of trust. I<br>
personally would recommend *not* upgrading to v0.10 due to these issues.<br>
<br>
A much safer approach would be to keep the code changes required for a<br>
consensus library to only simple movements of code for this release, accept<br>
that the interface to that library won't be ideal, and wait until we have<br>
feedback from multiple opensource projects with publicly evaluatable code on<br>
where to go next with the API.<br>
<br>
1) <a href="https://github.com/bitcoin/bips/blob/master/bip-0065.mediawiki" target="_blank">https://github.com/bitcoin/bips/blob/master/bip-0065.mediawiki</a><br>
2) <a href="https://github.com/bitcoin/bitcoin/pull/4890" target="_blank">https://github.com/bitcoin/bitcoin/pull/4890</a><br>
<span class="HOEnZb"><font color="#888888"><br>
--<br>
'peter'[:-1]@<a href="http://petertodd.org" target="_blank">petertodd.org</a><br>
00000000000000001b18a596ecadd07c0e49620fb71b16f9e41131df9fc52fa6<br>
</font></span><br>------------------------------------------------------------------------------<br>
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server<br>
from Actuate! Instantly Supercharge Your Business Reports and Dashboards<br>
with Interactivity, Sharing, Native Excel Exports, App Integration & more<br>
Get technology previously reserved for billion-dollar corporations, FREE<br>
<a href="http://pubads.g.doubleclick.net/gampad/clk?id=164703151&iu=/4140/ostg.clktrk" target="_blank">http://pubads.g.doubleclick.net/gampad/clk?id=164703151&iu=/4140/ostg.clktrk</a><br>_______________________________________________<br>
Bitcoin-development mailing list<br>
<a href="mailto:Bitcoin-development@lists.sourceforge.net">Bitcoin-development@lists.sourceforge.net</a><br>
<a href="https://lists.sourceforge.net/lists/listinfo/bitcoin-development" target="_blank">https://lists.sourceforge.net/lists/listinfo/bitcoin-development</a><br>
<br></blockquote></div></div>