Add height parameter to walletnotify message

[chromatic]

Use %i in the command to include the height of the block containing the transaction.

I found myself wanting this when trying to perform an action on an incoming transaction while awaiting n block confirmations. Otherwise I have to perform multiple RPC calls to get this information.

[sinetek]

looks good to me

[patricklodder]

Conceptually: how do you propose to detect when a chain tip fork happened and the transaction is no longer in the chaintip? This is important since after a period of great decline in fast nodes, and a subsequent month of spam, I've seen orphan rates of around 5%.

Completeness: I think this could use documentation on the -help text, in /src/wallet/wallet.cpp#L3662

[chromatic]

I think the existing mechanism already has the chain tip fork problem, so if we're going to solve that problem, we should solve it for block/wallet notification mechanisms in general.

I'll add the appropriate -help text.

[patricklodder]

I think the existing mechanism already has the chain tip fork problem

It does do something per line 958 of wallet.cpp, when the block hash gets unset: it broadcasts the transaction hash again in the current situation. That is not reflected in the code you added to look up the height though, so perhaps we ought to do something more there than just search for the blockhash?

[chromatic]

Oh, I see what you mean. In that case we have a couple of options:

• add an additional parameter to disambiguate new from updated
• skip the height on updated transactions
• use a flag value (-1? 0?) for updated transactions

I tend to prefer unambiguous data, so my slight preference is the first option. All of these are easy enough to code and explain.

[patricklodder]

I would recommend going for option 3 for now and use a static value for when wtxIn.hashUnset() is true, because we're dealing with a format string. Adding another formatting token will complicate stuff and invite for user omission/error.

-1 has the downside that implementors need to make their int signed. I think that 0 will do the job, because transactions (there is only one) in block 0 (genesis block) are unspendable.

[chromatic]

Something more like this you think?

int64_t nHeight = wtxIn.hashUnset() ? 0 : mapBlockIndex[wtxIn.hashBlock]->nHeight;

No extra parameter, no extra formatting token, present only when the user has asked for the height as a parameter, unambiguous and static flag value that I can document when I update -help?

[patricklodder]

I think that that is the simplest solution, yes. If you think there is an easier way, I'm open, but I think this is better than omitting the parameter, and less complicated than adding another optional token.

[chromatic]

Squashed and rebased.

[patricklodder]

tested ACK, but with a discovery:

When testing, I found that when reversing blocks that contains a transaction, the wallet sends out 2 notifications.

Test script: https://github.com/patricklodder/dogecoin/blob/1.14.7-walletnotify-test/qa/rpc-tests/walletnotify.py
Results:

$ python3 qa/rpc-tests/walletnotify.py 
Initializing test directory /tmp/test1ytxzzlj/926722
Notifications after reversing:
  d2d825f1cc24e7be6a27e44d11922f17de8ceac833c6ade1b62097b4018584e1 0
  d2d825f1cc24e7be6a27e44d11922f17de8ceac833c6ade1b62097b4018584e1 221
  d2d825f1cc24e7be6a27e44d11922f17de8ceac833c6ade1b62097b4018584e1 0
  d2d825f1cc24e7be6a27e44d11922f17de8ceac833c6ade1b62097b4018584e1 0
  
Assertion failed: 
  File "qa/rpc-tests/test_framework/test_framework.py", line 145, in main
    self.run_test()
  File "qa/rpc-tests/walletnotify.py", line 95, in run_test
    assert len(notifs) == self.current_line + 2
Stopping nodes
Not cleaning up dir /tmp/test1ytxzzlj/926722
Failed

I'm okay with not fixing it in this PR (exceeds scope of this change) but we will need to figure out why it does that, and either fix or document the behavior.