Right Work

Right Work

This post is for anyone who wishes they could contribute to Bitcoin-ABC but doesn’t really know where to start. If that’s you, then you should start with what Andre Alexandrescu calls “right work.” The linked post was recently added to the Bitcoin-ABC Contributing Guide.

The gist was that there are several important classifications of open source contributions:

1. Great work – It solves a problem elegantly and simply. The solution does not add a disproportionate amount of complexity to the codebase.
2. Good work¹ – This type of work should be avoided; it is often deceptively bad. Although it may add some useful feature, it often is:
   • Large and complicated for the benefit it brings which potentially introduces bugs.
   • Time needs to be spent convincing people the change is a good idea.
   • The quality needs to be improved, so careful review and multiple cycles of review are required.
   • Drains resources away from the maintainers who must deal with the above.
3. Bad work – Easy to identify as wrong, and reject. This doesn’t take much time from the maintainers, but is often frustrating to the person trying to contribute
4. Right work – Here Dr. Alexandrescu uses a lot of examples specifically relating to the D programming language and its standard library and compiler frontend. The changes are easy and fast to review. Also, the more right work is done, the more likely Great Work can be done easily in the future.

There is a huge and undersupplied demand for Right Work in the world of software engineering, and especially open source. The reason for this is that right work does not result in any directly visible impact to the user² – no bug is fixed, nor feature added. The only discernible impact is that future work on the codebase is easier. Unfortunately, other engineers are often the least likely to give praise, even for this type of work.

But what exactly is “right work”? Right work, simply put, is cleaning up small messes. Now, you may think that if every new patch that went into a project was perfect, that the project as a whole would never need any “right work”. However, bitcoind is a relatively old codebase that has had lots of contributors. Often with open source projects, authors come in and add some functionality and then leave. They might have done a great job, but then later another person will come and remove or alter the functionality. Possibly because it is no longer useful, or maybe they generalize it. Unfortunately, it is easy to leave unnecessary code behind which incurs a maintenance cost – although it may provide no function to the software.

Another common occurrence in open source is that someone, needing to add functionality, does so with the minimum amount of effort. They add some global in one part of the code, an additional if statement somewhere else, and call it good. Over time, this results in an explosion of complexity from too much “action at a distance” in the code. This makes it very hard to reason about the code, and to make sure that it is thread safe.

Often, code is moved or changed, but “comment blindness” causes comments to not be updated, resulting in confusion down the road.

There are many other phenomena that happen from code churn in open source projects. The disorder that slowly grows in a codebase makes it progressively more challenging to reason about, and difficult to reliably change. Slowly, the code ossifies – especially in the case of Bitcoin where bugs are catastrophic. Most engineers contributing to open source do so only to fix a bug, or add a feature, that is directly impacting them. Very few contributors want to take on the role of Maxwell’s Daemon and start tidying up a code base³.

Rarely are examples of “right work” highlighted over “great work” or “good work”. This is unfortunate, because it sets the wrong precedent for what work people should strive toward. “Right work” is a great exercise for learning the codebase, and also provides a lot of value to software engineers who come after. The best thing about right work is that almost anyone can do it.

So, here’s some major classes of types of “right work” I’ve seen in Bitcoin-ABC, and examples thereof:

Maintaining build and test infrastructure

• Switch to GCC 7 on Travis, disable currently non-functional ARM + Windows builds

Fix major usability bugs

• Support for encoding addresses using cashaddr - Dagur Valberg Johannsson

Remove unnecessary dependencies

• Replace boost::unordered_map and boost::unordered_set with std:: variant - Awemany
• Removed all usages of boost::assign::list_of in favor of C++11 initializer lists - Calin Culianu

Delete unused code

• Get rid of SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_WITNESS_PROGRAM – Andrea Suisani
• Fix undefined behaviour errors due to long int overflow in Amount - Fabien
• Delete block policy estimator
• Remove much of the remaining BIP9 code
• Removed unused parameter from validFeeEstimates
• Remove unused parameter from AcceptBlock
• Remove extraneous parameter from PrioritizeTransaction
• Remove unused fAllowFree variable from coincontrolwallet.cpp
• Remove IsPayToWitnessScriptHash – Andrea Suisani
• Remove incremental relay fee
• Cleanup configure.ac to remove old boost versions workarounds - Fabien
• Remove unused seeder/compat.h - Fabien

Delete duplicate code

• Remove duplicated code which can be obtained by linking crypto lib
• Remove a ton of code from seeder’s util - Amaury Séchet

Fix warnings

• Fix a bunch of clang warnings
• Fix a Clang -Wshadow warning in rcu.h - Fabien
• Fix clang warnings - Fabien
• Fix some compiler warnings regarding override

Update code to latest standards

• Use static_cast instead of C-style casts for non-fundamental types - Nico Guiton

Fix flaky tests

• Fix sporadic policyestimator_tests failure due to unspecified operand evaluation order – Freetrader
• Remove redundant connect_nodes call in abc-parkedchain.py – Jason Cox

Remove deprecated functionality

• Remove deprecated estimateSmartPriority function

Refactor for dependency injection

• Make Config more available in Qt - Dagur Johannsson
• Accept Mempool as a parameter to CBlockAssembler

Remove global variables

• Remove minTxFee from wallet
• De-globalize blocksize related parameters
• De-globalize rpc configuration variables
• Move minimum relay fee
• Move handling for -maxtxfee to the config object

Make code more legible

• Removed using namespace std and replaced with std:: throughout file - gbrown
• Use std for make_pair in walletdb.cpp – Freetrader
• Remove blockFinished from BlockAssembler
• Clean up addPriorityTxns
• Typedef for TXmodifiers

Make the code type safe

• Add Amount class and use it for CENT and COIN

Read and fix/add comments

• Comment, cast and assert cleanup in pow.cpp - Freetrader
• Fix comment in CheckInputs to match changed code
• Make comments in abandonconflict legible

Add tests/Fix Tests

• Add ATMP test for undersize transaction - Dagur Johannsson
• Fix tx padding – Dagur Johannsson
• Fix p2p-fullblocktest to pad transactions properly - Jason Cox
• Add a test case to ensure mempool accounting is correct
• Check that key length is correct when deserializing CExtKey - Amaury Séchet – Add test for extPubkey
• Add test cases to test new bitcoin-tx functionality - John Newberry
• add testcases for getrawtransaction - John Newberry
• Lots of other great work from John Newberry

Organize source files

• Move block undo out of core – Jorge Timón
• Move network processing code out of main.cpp – Matt Corallo
• Move CFeeRate into its own source files
• Removed CBlockStatus class from chain.h and placed into its own .h file - Nico Guitan

Update packaging

• Ubuntu PPA Maintenance by Andrea Suisani
• Dockerfiles by Josh Ellithorpe
• Update various debian packaging files – Freetrader
• OSX: Change app bundle name and executable name to BitcoinABC-Qt.app and change related OSX-specific handlers and Info.plist stuff to match. – Calin Culianu

Read and subscribe on Substack