Ralph Waldo Emerson famously said “To be great is to be misunderstood” and “With consistency a great soul has nothing to do.”
The mature programming world has a principle called Don’t Repeat Yourself or DRY. The idea there is to find and eliminate insidious duplication in your system.
What’s insidious duplication? Let me tell you a story.
One of These Things is Not Like the Other
In my work to make Dogecoin Core’s RPC and GUI interfaces more pleasant to use, I’ve been working to do less work, so users can get back to what they want to do and spend less time and energy waiting for unnecessary things to do nothing.
In practice, that means going through all of the RPC methods and GUI mechanisms that could lead to scanning the entire blockchain or transaction history or other big lists of things and giving users the power to limit the thing being scanned.
Enter PR 3102. The
importpubkey RPC command lets you add a public key to your core wallet. If
you know this address has received transactions before, you can ask the core to
rescan all transactions to see what’s in the wallet now.
Your Spidey-sense should tingle at reading that, because mine did. That’s a perfect place to make the behavior of this command consistent with other RPC commands.
That’s what this PR does; it adds an optional height parameter so that you
can say “scan only from block at height $X” and avoid rescanning blocks before
anyone ever used that address.
So far so good.
This should have been easy. I’ve written PRs like this before. A little copy and paste and tweak and….
When Orange Rhymes with Porridge
If you look carefully at the PR, you’ll see I created a new function called
attemptRescanFromHeight that takes the data structure containing the RPC
command request, looks in that request for a height parameter, and attempts
to rescan the wallet from that height.
This makes sense. We want similar behavior for scanning the blockchain when we import both public and private keys.
Similar, or identical? Great question!
I extracted this code from both RPC commands, but the code to import a public
key had additional behavior: a call to a wallet method named
ReacceptWalletTransactions.
That sounds useful and suspicious. Why was it there and not in the other method?
I don’t actually know.
If you browse the history of the Core code, that method exists in a form not entirely unlike its current form in Satoshi’s original commit to the original Bitcoin codebase. In my spelunking, I’m pretty sure it adds transactions to the wallet if those transactions are only in the mempool, but I’m not sure why it’s in one command and not the other.
If I were writing this code, I’d put it in both commands or neither, and if it had to be in one but not the other, I’d add a comment as to why.
What’s the right approach in this case?
Make Things the Same, then Make Things the Same
I forget where I read it, but it was probably an interview with Ward Cunningham where he said “When I come to refactor things that aren’t the same, first I make them the same, then I refactor the duplicate pieces out of them”.
That’s what I did here.
This PR hasn’t been accepted yet. It’s probably largely correct, but I don’t
have a great explanation for why something I expected to be duplicate code
wasn’t actually duplicate code. If you read other RPC commands in
src/wallet/wallet.cpp, they also call ReacceptWalletTransactions after
rescanning the blockchain, so I’m sure I’m on the right track.
At worst, we’ll all learn something about what’s different between things that seem like they should be the same, and at best, we’ll have made the Core a little easier to use, made some mostly-duplicate code actually duplicate and then not repeated, and learned something.
Even a task that seems as simple and small like what I set out to do here can become an opportunity to learn something and to find a bigger improvement lurking elsewhere. That’s part of what makes this work interesting.
