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.