Implement and enforce standards-requirements for critical bugfixes.

[vinniefalco]

Currently, critical bugfixes are being committed into the repository with little to no documentation. A critical bugfix is defined as a source code change which solves a problem with the business logic reported by users or partners, or observed by developers. Business logic is defined as those parts of the program that are specific to the domain (payments). This includes transactions, the ledger, historical data, order books, account state, and the consensus process.

Here are some examples of critical bugfixes that have almost no supporting documentation:

Don't allow a payment to take anything from an expired offer. https://github.com/ripple/rippled/commit/46861fac485b8e958b61f0299f68d2117d844428

Fix a race condition if PathRequest::doCreate races with the path being processed. https://github.com/ripple/rippled/commit/4620b667e729335778e13ec54cb97f50be7a73e5

Allow two trust lines to be created without reserve check. https://github.com/ripple/rippled/commit/c211094d3e710f4b9d7d85c98b40e58c1204d315

We propose the following changes in the workflow for submitting critical bugfixes to rippled:

1. A comment describing the bug along with supporting documentation (such as a link to the report or issue) shall be placed near the faulty code when such an area is identified as flawed.
2. An additional comment explaining how the bug is manifested shall be placed near the faulty code. This includes any relevant information such as the state of variables that cause the defect, or how to reproduce the defect easily.
3. An additional comment describing the specific business rules that are applicable to the faulty code shall be placed near the affected area. A link or section number from some supporting document (such as a "rippled programmer's documentation" inlined in the repository) would be sufficient,
4. A brief comment explaining the fix. Examples include:

// Skip the remainder of the loop body since we no longer wish
// to process this possible path.

// The comparison against the number 2 allows the first two trust
// lines to be free, removing any incentive for gaming new accounts.

Additionally, whenever it is feasible to do so, or when such time arrives that existing business logic can be suitably isolated, we recommend the following:

5.. A unit test be added to the bottom of the corresponding source file that reproduces the conditions under which the original defect occurs, and verifies that the defect is no longer present.

We note that bug fixes comprised of changing or inserting a single character are inexplicably committed to the repository with not even a comment. For example:

Fix the path filtering loop exit condition. https://github.com/ripple/rippled/commit/0ae7bcff5250e8a91320193a55ac15978537ed74

In addition to having well-defined, written business rules for structuring bugfix commits, we should also define a process for enforcing the rules.

[nbougalis]

Absolutely 100% agree with all the above.

[sublimator]

It seems ripple still needs a huge amount of work to get stable, bug free and fit for purpose (global payment network scalable to processing trillions of transactions and supporting hundreds of millions of users) A long road lays ahead and getting to the end will require more programmers, which in turn requires an approachable, comprehensible code base. Given the financial application of the project, better exposition of ripple concepts through comments and documentation, and verification through tests seem a must (in rippled/ripple-lib-*)

[kravets]

In complete agreement here with what @vinniefalco and @sublimator said. It will be difficult to convince institutions to adopt Ripple while it's hard for their staff C++ programmer to verify what rippled is claiming to be doing and whether it actually does what it claims. Better names, more disclosure of the intent behind various conditionals, etc would all go along way to build confidence. It should be possible to do a module-by-module divide-and-conquer audit of the code base by a team of competent C++ programmers inside a Big Bank.

[vinniefalco]

Obsolete