Triaging of existing TODOs/FIXMEs

[JoeLoser]

From the suggestion by @scottschurr in https://github.com/ripple/rippled/pull/2607, I've gone ahead and done an audit of existing TODO and FIXME notes and downselected some from there to post some here. The hope is to get someone on the team with some more insight on these issues to do an initial triaging to form a taxonomy of the below items, something similar to what Scott suggests:

1. Stale and the note should simply be removed from the codebase.
2. Good things to fix and probably don't have lots of tricky parts.
3. Good things to fix, but look nasty and complicated.

I plan on using this taxonomy to help guide what I could work on next.

---

1. https://github.com/ripple/rippled/blob/develop/src/ripple/protocol/impl/SecretKey.cpp#L68
2. https://github.com/ripple/rippled/blob/develop/src/ripple/protocol/impl/STTx.cpp#L403

• Allow memo limit to be a config parameter

3. https://github.com/ripple/rippled/blob/develop/src/ripple/app/misc/NetworkOPs.cpp#L2953

• Do we want the new or old code? Does anyone actively use this macro toggle currently? @nbougalis

4. https://github.com/ripple/rippled/blob/develop/src/ripple/app/consensus/RCLConsensus.cpp#L571

• Use median instead of avg for close time with other nodes

5. https://github.com/ripple/rippled/blob/develop/src/ripple/app/paths/Pathfinder.cpp#L823
6. https://github.com/ripple/rippled/blob/develop/src/ripple/overlay/impl/PeerImp.cpp#L349
7. https://github.com/ripple/rippled/blob/develop/src/ripple/app/paths/cursor/Liquidity.cpp#L57

• Similar note in https://github.com/ripple/rippled/blob/develop/src/ripple/app/paths/cursor/ReverseLiquidityForAccount.cpp#L155

8. https://github.com/ripple/rippled/blob/develop/src/ripple/app/paths/cursor/AdvanceNode.cpp#L102

• Better TER return code for case of no more offers

9. https://github.com/ripple/rippled/blob/develop/src/ripple/app/tx/impl/Transactor.cpp#L356

• Better return code for case of unknown signing public key type

10. https://github.com/ripple/rippled/blob/develop/src/ripple/basics/StringUtilities.h#L33

• Perhaps just replace with Boost.Hex and write thin wrappers where appropriate.
• Could possibly remove some of https://github.com/ripple/rippled/blob/develop/src/ripple/basics/strHex.h

11. https://github.com/ripple/rippled/blob/develop/src/ripple/protocol/STObject.h#L365
12. https://github.com/ripple/rippled/blob/develop/src/ripple/protocol/STObject.h#L378
13. https://github.com/ripple/rippled/blob/develop/src/ripple/rpc/handlers/ValidationSeed.cpp#L36

• Should we just remove this feature? It seems to be disabled since 2015.

14. https://github.com/ripple/rippled/blob/develop/src/ripple/app/ledger/impl/LedgerCleaner.cpp#L270

Thanks for the help!

[scottschurr]

I took a quick glance at a few of these. A number are in areas I'm not comfortable talking about without a lot of study. But I'll hazard a thought on a couple right off the top:

2. Allow memo limit to be a config parameter.

This comment strikes me as dead wrong. A memo is part of a transaction. The entire network needs to agree on the validity of a transaction. Having different network nodes set different limits on the size of a memo seems completely improbable. But the original commenter may have had something in mind that I don't understand.

My personal take, without more background, is that the comment should be removed.

9. Better return code for case of unknown signing public key type.

I don't have a lot of sympathy for this FIXME. An unknown key type is just a special case of a corrupted key. If the corruption is in the right place we can't determine the key type. If the corruption is in a different place, then it's just an ill-formatted key. My personal take is that any of them are just bad keys and deserve a similar response.

My personal take is that the comment should be removed.

12. Json::Value getJson() options should be an enum

Yup, I whole-heartedly agree with this TODO. It will take some spelunking to discover the right values for the enum. Most calls to getJson() pass a zero and the callee ignores it. Occasionally I see non-zero values being passed (usually a 1), and I'm puzzled by what the difference is. To solve this well you'll need to find all the places where a non-zero is being passed and find out what the consequences are.

You'd be doing two good things. 1) Characterizing the options that getJson() is currently using. 2) Giving those options readable names so readers of the code know what is being asked for.

14. getLedgerHash() should return a boost::optional<uint256>

I only half-heartedly agree with this TODO. For me, at least, I'm puzzled why LedgerHash is just an alias (https://github.com/ripple/rippled/blob/develop/src/ripple/protocol/RippleLedgerHash.h#L27) instead of a tagged base_uint, like these: https://github.com/ripple/rippled/blob/develop/src/ripple/protocol/UintTypes.h#L50-L58.

So my take would be to start by seeing how much trouble we get into by turning LedgerHash into a tagged uint256. That feels like the more beneficial change. But it's always possible something will get messy if you try it. The good news is there are three pre-existing tagged base_uints. You can look at usages of those for examples if you get into trouble.

Once that is done, then changing the return type of getLedgerHash() is a lower-value change. And that change should be made in the context of other functions/methods named getLedgerHash(). Personally, I find having things with the same names return different types to be a source of error and confusion (especially in a world with auto). So I think it would be good to look at everything named getLedgerHash() and make a judgement call given that list.

On a related note, it looks like https://github.com/ripple/rippled/blob/develop/src/ripple/basics/tagged_integer.h may be an (almost) unused file? You could potentially look into that and see if we could yank that file.

I hope that helps. Hopefully a few other folks will chime in on some of the other places you identified.

Thanks for your contributions!

[JoeLoser]

@scottschurr very helpful! Thanks for the response. Sounds like (12) and (14) are worth digging into after the holiday, so I'll at least start there. Maybe others will have some recent experience in the other areas and can chime in there so we can prioritize appropriately.

[nbougalis]

14. getLedgerHash() should return a boost::optional

only half-heartedly agree with this TODO. For me, at least, I'm puzzled why LedgerHash is just an alias (https://github.com/ripple/rippled/blob/develop/src/ripple/protocol/RippleLedgerHash.h#L27) instead of a tagged base_uint, like these: https://github.com/ripple/rippled/blob/develop/src/ripple/protocol/UintTypes.h#L50-L58.

I'd like to see a tagged base_uint used as well. :+1:

On a related note, it looks like https://github.com/ripple/rippled/blob/develop/src/ripple/basics/tagged_integer.h may be an (almost) unused file? You could potentially look into that and see if we could yank that file.

It is unused (except for some tests, and it's functionality there is dubious). The class can allow for stronger type safety (e.g. distinct "ledger sequence number" and "account sequence number" types, both of which are, std::uint32_t internally) but I'm not sure that it's worth keeping this code.

[nbougalis]

1. https://github.com/ripple/rippled/blob/develop/src/ripple/protocol/impl/SecretKey.cpp#L68

I wouldn't bother with this, to be honest. It's likely that this code will be cleaned up soon in a much bigger way, and this will be handled then.

[nbougalis]

4. https://github.com/ripple/rippled/blob/develop/src/ripple/app/consensus/RCLConsensus.cpp#L571 Use median instead of avg for close time with other nodes

A couple of comments about this. We contemplated fixing this for 1.1.0 but decided to punt.

Relevant questions are does it make sense to change the calculation, and if so, is the median the best measure of central tendency? My first intuition is that mode is probably better in this case ("go with the most popular close time"), but it has its own set of issues (think: 4 different close times, each supported by the same number of votes).

I guess I can get behind the median, but we'll need to be careful in how we calculcate it; right now, our mean calculation breaks the type system; I think it's possible to do all this entirely within the std::chrono framework, but I'd defer to Mr. Chrono himself, @HowardHinnant.

[nbougalis]

13. https://github.com/ripple/rippled/blob/develop/src/ripple/rpc/handlers/ValidationSeed.cpp#L36 Should we just remove this feature? It seems to be disabled since 2015.

This is taken care of with #2734. Good spotting.

[carlhua]

Re-evaluate the action items listed in this issue.