wallet: Add new format string placeholders for walletnotify #21141
Conversation
maayank
changed the title
maayank
force-pushed
the
walletnotify-blockhash
branch
from
5c08825
to
00fd193
Compare
|
Thank you for the comment @promag . It's an important comment especially for future readers intending to use it for fork detection. (not related) I've changed the code to use ToString instead of std::to_string to avoid locale linter errors. What do you say on changing the start of the description "Execute command when a wallet transaction changes" to "Asynchronously execute command when a wallet transaction changes"? It's true also before this commit. Would help to highlight potential tradeoffs for future users. |
|
Concept ACK, I don't think adding this information to notifications can hurt and it's only very little more code. |
maayank
force-pushed
the
walletnotify-blockhash
branch
3 times, most recently
from
4a04028
to
3d0ee35
Compare
|
I've noticed in the CI checks some linter errors as well as that on some systems the notification test case failed due to the concatenation of wallet, txid, block height and block hash being too long of a filename. I've changed the test case so it now checks not only the filename (for wallet and txid) but contents (for block height and block hash). Now the request passes all the CI tests and is ready to merge. |
|
Concept ACK |
maayank
force-pushed
the
walletnotify-blockhash
branch
from
fa9a216
to
0832edc
Compare
|
Agree with @luke-jr 's suggestions, ACK after squashing commits. |
maayank
force-pushed
the
walletnotify-blockhash
branch
2 times, most recently
from
9ad59c9
to
4d6b785
Compare
…nt block information for transactions
maayank
force-pushed
the
walletnotify-blockhash
branch
from
4d6b785
to
06e1fb0
Compare
|
ACK 06e1fb0 |
| @@ -944,6 +944,14 @@ CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const CWalletTx::Confirmatio | |||
| if (!strCmd.empty()) | |||
| { | |||
| boost::replace_all(strCmd, "%s", hash.GetHex()); | |||
| if (confirm.status == CWalletTx::Status::CONFIRMED) | |||
| { | |||
There was a problem hiding this comment.
- if (confirm.status == CWalletTx::Status::CONFIRMED)
- {
+ if (confirm.status == CWalletTx::Status::CONFIRMED) {There was a problem hiding this comment.
Oops, noticed this too late, sorry
…alletnotify 06e1fb0 Add new format string placeholders for walletnotify to include relevant block information for transactions (Maayan Keshet) Pull request description: This patch includes two new format placeholders for walletnotify: %b - the hash of the block containting the transaction (zeroed if a mempool transaction) %h - the height of the block containing the transaction (zero if a mempool transaction) I've included test suite changes to check and validate the above functional requirements as well as doc/help description changes. **Motivation** The walletnotify option is used to be notified of new transactions relevant to the wallet of the node. A common usage pattern is to perform afterwards additional RPC calls to determine: 1. If this is a mempool transaction or not (i.e. are there any confirmations?) 2. What block was it included in? 3. Did this transaction was seen before and is now seen again because of a fork? All of these questions can be answered with the current features, but the resulting RPC calls may be expensive in a heavily used node. As this information is readily available when calling the walletnotify callback, it makes sense to save expensive round trips by optionally sending this information at that point in time. I can definitely say we would like to use it in Fireblocks, my employer. Please let me know of any questions and suggestions. ACKs for top commit: laanwj: ACK 06e1fb0 Tree-SHA512: d2744e2a7a883f9c3a9fd32237110e048c4b6b11fea8221c33d10b74157f65bbc4351211f441e8c1a4af5d5d38e2ba6b1943a7673dc18860c0553d7b41e00775
…nt block information for transactions Github-Pull: bitcoin#21141 Rebased-From: 06e1fb0
This patch includes two new format placeholders for walletnotify:
%b - the hash of the block containting the transaction (zeroed if a mempool transaction)
%h - the height of the block containing the transaction (zero if a mempool transaction)
I've included test suite changes to check and validate the above functional requirements as well as doc/help description changes.
Motivation
The walletnotify option is used to be notified of new transactions relevant to the wallet of the node.
A common usage pattern is to perform afterwards additional RPC calls to determine:
All of these questions can be answered with the current features, but the resulting RPC calls may be expensive in a heavily used node. As this information is readily available when calling the walletnotify callback, it makes sense to save expensive round trips by optionally sending this information at that point in time. I can definitely say we would like to use it in Fireblocks, my employer.
Please let me know of any questions and suggestions.