Add option to only notify after wallet transactions are confirmed

skeees commented at 7:22 pm on March 27, 2018: contributor

Optionally only call -walletnotify command when a certain number of confirmations have been seen

This addresses issue #10021

laanwj added the label Wallet on Mar 27, 2018

skeees force-pushed on Mar 27, 2018

promag commented at 9:36 pm on March 27, 2018: member

NACK at quick glance. Why add complexity when it can be done outside? What happens to the queue when it restarts? Are the notifications lost? I don’t think we should track state on behalf of the client.

skeees commented at 9:46 pm on March 27, 2018: contributor

re recovering state upon restart - if this option is enabled the wallet rescans the last n_confirmations_required blocks to repopulate this queue (see link below): the expectation is that the number of confirmations desired is a number closer to 6 than to 100k - so the rescan shouldn’t add tremendous overhead. Of course, the number of confirmations desired is up to the user, and is off by default. https://github.com/bitcoin/bitcoin/pull/12801/commits/d232f62462c6cebf5e03b7cbe7c87469f46ca026#diff-b2bb174788c7409b671c46ccc86034bdR4072

re the NACK - @laanwj expresses some support for putting this functionality into the daemon in #10021

ryanofsky commented at 10:01 pm on March 27, 2018: member

@promag this seems like it could be significantly more efficient than the suggested workaround from #10021 (comment) of calling listtransactions. Or did you have a different workaround in mind?

From the original issue, it sounds like lack of a configurable notifications gives wallet users an incentive to only wait for a single confirmation, which seems not great.

As for code complexity, I’d have to look more closely, but the new logic might be reusable to update displayed transactions in the gui more efficiently, and the state tracking might be useful in the wallet itself to get rid of some synchronous chainactive lookups.

Add option to only notify after wallet transactions are confirmed

Adds option: -walletnotifyconfirmations=<n> (default: 0) which
works in conjunction with walletnotify=<cmd>
Default value of 0 maintains existing behavior: notification will occur every time a wallet transaction is changed
If set to n >= 1 then notification will occur at most once after a transaction
is confirmed by n blocks

3a4dd72d42

skeees force-pushed on Mar 28, 2018

skeees commented at 2:27 pm on March 28, 2018: contributor

Rebased to latest master (should solve test failures)

promag commented at 8:46 am on March 30, 2018: member

I’ll review and test. I understand this feature is very desirable as it can save a lot of code for clients that want the Nth confirmation.

Remove redundant log and fix off-by-one 8afcf1fa91

fix flake8 F841 unused variable warning in functional test 23366c342d

in test/functional/feature_notifications.py:107 in 8afcf1fa91 outdated

102+        # mine some more blocks (we should receive notifications on the ones above plus all but the last n_confirmations - 1 of these)
103+        generated_after_restart = self.nodes[1].generate(block_count + n_confirmations - 1)[:-n_confirmations + 1]
104+        wait_until(lambda: os.path.isfile(self.tx_filename) and os.stat(self.tx_filename).st_size >= (block_count * 65), timeout=10)
105+
106+        # get the list of block hashes that should now be confirmed
107+        buried_blocks = generated_before_restart + generated_after_restart

jamesob commented at 7:37 pm on April 3, 2018:

Unused symbol (causing the Travis failure). You can catch stuff like this locally by running ../../contrib/devtools/lint-python.sh from within test/functional.

skeees commented at 0:16 am on April 4, 2018:

Thanks! Updated

in src/wallet/wallet.cpp:962 in 23366c342d outdated

965-    {
966-        boost::replace_all(strCmd, "%s", wtxIn.GetHash().GetHex());
967-        std::thread t(runCommand, strCmd);
968-        t.detach(); // thread runs free
969-    }
970+    if (m_notifier.GetConfirmationsRequired() == 0)

jamesob commented at 6:13 pm on April 4, 2018:

Nit: braces (per dev notes)

in src/wallet/wallet.cpp:1204 in 23366c342d outdated

1198@@ -1206,6 +1199,10 @@ void CWallet::SyncTransaction(const CTransactionRef& ptx, const CBlockIndex *pin
1199             it->second.MarkDirty();
1200         }
1201     }
1202+
1203+    // if we are notifying in confirmed mode and this was due to a connected block
1204+    if (pindex && m_notifier.GetConfirmationsRequired() >= 1)

jamesob commented at 6:13 pm on April 4, 2018:

Nit: braces (per dev notes)

in src/wallet/wallet.h:689 in 23366c342d outdated

684+
685+    /*
686+     * Should be called every time a block is connected
687+     *
688+     * When m_confirmations_required > 1, notifies
689+     * of transactions that have not reached the

jamesob commented at 6:41 pm on April 4, 2018:

Should this be “that have reached” vs. “that have not reached”?

jamesob commented at 6:50 pm on April 4, 2018: member

Concept ACK

I like the feature and the tests, but this implementation seems a little complicated to me. Maybe I’m just dense, but I have a hard time reasoning about the use of the circular buffer, buffer_pos-based indexing, etc.

Would it be simpler to have WalletTransactionNotifier maintain a std::map<int, std::vector<CTransaction>> target_height_to_txns where the key is the block height to notify at and the value are the txns to notify on? That way in WalletTransactionNotifier::BlockConnected, all you’d have to do is iterate through the map and evict/notify anything with a key less than or equal to the current block height. An added benefit of this approach is that you no longer have to worry about BlockConnected being reliably called for each block in order to maintain correctness.

skeees commented at 8:33 pm on April 4, 2018: contributor

(will address other comments, re: map/vec) The tricky bits here involved being correct around chain re-orgs and I don’t think a map makes that any less complex. It also doesn’t let you discard the dependency on reliable invocation of the BlockConnected signal without introducing a lot more state, inefficient chain scanning, or discarding some of the guarantees about notification:

Here’s an explanation of the circular buffer approach.

n: the number of confirmations needed for notification to occur

When n==1 we notify as soon as the block is mined so no state is needed.

When n>1 we need to keep around a state representing blocks with transactions with haven’t yet been confirmed: there will be at most n-1 of these.

Since n is generally chosen to be a small number - and also since any node operating for sufficiently long time to observe n blocks will eventually have to store state of length n-1 - we just pre-allocate a buffer with n-1 slots. Most of the time (except for re-orgs) we will be adding a new highest height (new block) and removing the lowest height (block that has now received n confirmations and we’ve notified) - this is a natural fit for a circular buffer.

buffer_pos(height): maps every block height into a slot in this buffer with simple modulo arithmetic

If this buffer is reliably updated every time a block is connected, then it will always contain valid state for up to the n-1 blocks on the active chain tip. Up to, and usually, but not always exactly, n-1 because of re-orgs. In a reorg when the first block on the newly reorged chain gets connected at a height lower than the tip, we do not need to rescan any of the trailing blocks for which we no longer keep state because those have already been appropriately notified previously. We do have to maintain some bounding logic to track min/max height ranges for which we have valid state lest we accidentally send notifications for transactions in higher blocks on the re-orged out chain (b/c of the modulo based indexing into the circular buffer). With a map - you wouldn’t need these min/max height bounds - but you trade that for having to explicitly clear the map for heights that are no longer needed.

In no case (map or vector) are you able to relax the constraint that BlockConnected must be called reliably without adding either more complex state (e.g. block hashes so you don’t accidentally notify for re-orged out blocks - you won’t send false notifications but you might miss some) or expensive scans of trailing blocks on every update (you wouldn’t miss blocks - but it’s quite inefficient and you’d either have to rescan the entire chain on startup or persist your last notified height to disk). Also there are a number of other internal components that need to have validation signals invoked reliably - so its not a new dependency.

The only other savings in logical complexity you get by substituting a map for a pre-sized vector is indexing into the structure (i.e. map[height] instead of vec[buffer_pos(height)].

One other note here - this guarantees “at least once” notifications. Duplicate notifications can occur when the same blocks are connected more than once (e.g. on node restarts or chain re-orgs).

Address review comments c7bee6f9c3

jnewbery commented at 6:14 pm on April 9, 2018: member

Apologies for only coming to this after it’s already received some review, but I’m a concept NACK on this, and think that the method suggested here: #10021 (comment) is a good solution to this problem.

Adding this to the server seems to be adding a lot of complexity for very little gain. There are edge cases to consider (eg transactions getting removed in re-orgs, or added at a different block height), and maintaining/debugging issues with this quite complex server code seems to be a high price to pay for something that can be achieved on the client side.

jamesob commented at 6:28 pm on April 9, 2018: member

Maybe a moot point now given some concept NACKs, but the implementation as written is faulty for a particular reorg case (failing test here). Feel free to cherry-pick that commit should work on this continue.

I appreciate the good explanation of your initial implementation @skeees, but I still think this approach is overly complicated and too easily hides faulty behavior. I think if work continues on this feature, we should use something along the lines of a target_height_to_txns map in conjunction with ValidationInterface::BlockDisconnected to clear keys and thus more explicitly handle reorgs. I think that’d be a bit easier to reason about.

Add reorg test case (from: @jamesob) 6163f6d188

skeees commented at 0:02 am on April 10, 2018: contributor

Thanks for the test case! The cause is rather embarrassing - due to my lack of familiarity with c++ I was calling std::vector::empty() (a boolean test for emptiness) instead of std::vector::clear() (actually empties the vector). I’ve fixed that and incorporated your test @jamesob and (slightly tweaked so it tests re-orgs at multiple depths)

Re the concept NACK in general - if reliance on zeroconf transactions is heavily discouraged then I think its certainly worth maintaining this in the wallet. Furthermore - as @ryanofsky has pointed out in earlier comments - there are a number of other places where this sort of state tracking would benefit the wallet and for which this logic could be further utilized. One example is coinbase maturity testing which currently calls GetDepthInMainChain(). I’d also note that while the logic added might be internally complicated - there is no additional persistent state introduced and the public interface to the class is very limited - i.e. I tried to minimize as much as possible how it interacts with the rest of the wallet (i.e. most notable of these was maintaining additional internal range bounding logic in exchange for not requiring notification on BlockDisconnected() events).

jamesob commented at 1:03 am on April 10, 2018: member

Ironically the one provision that tends me towards a concept ACK doesn’t jibe with reusing this sort of logic within parts of bitcoind, e.g. GetDepthInMainChain().

With the merge of #10244 we’re finally making concrete steps towards gui/wallet/node separation. I think it’s fine for the wallet to encompass useful (if nonessential) features like this, especially once we have firmer separation between the three major subsystems, but I think if we were to use this class as-is in something like coinbase maturity checks and in doing so introduce a bidirectional dependency between the wallet and the node, that’d be a step backwards.

Unless there was a really compelling case for this pattern to be used in bitcoind (and subsequently moved there and then exposed over an interface), I think it should stay completely isolated to wallet if implemented at all.

This seems like a nice feature for some users and I’m generally for it, but I’m also okay waiting on it until we get further down the road with node separation.

Any chance you can just cherrypick https://github.com/jamesob/bitcoin/commit/92b620f26127b93f2b6fd389516f5e7b7d09ac7e so that the commit attribution remains intact? You can then do git commit --amend to make your changes.

skeees commented at 1:14 am on April 10, 2018: contributor

Sorry if I was unclear, but I think we’re on the same page. What I was trying to say was with this sort of state in wallet, you wouldn’t need to rely quite as much on GetDepthInMainChain() which should further the goal of separation.

Yes, of course I’ll amend the commit as you suggest - did not realize I could do that – along with c++, I learn new things about git every day.

On Mon, Apr 9, 2018, 9:04 PM jamesob notifications@github.com wrote:

Ironically the one provision that tends me towards a concept ACK doesn’t jibe with reusing this sort of logic within parts of bitcoind, e.g. GetDepthInMainChain().

With the merge of #10244 https://github.com/bitcoin/bitcoin/pull/10244 we’re finally making concrete steps towards gui/wallet/node separation. I think it’s fine for the wallet to encompass useful (if nonessential) features like this, especially once we have firmer separation between the three major subsystems, but I think if we were to use this class as-is in something like coinbase maturity checks and in doing so introduce a bidirectional dependency between the wallet and the node, that’d be a step backwards.

Unless there was a really compelling case for this pattern to be used in bitcoind, I think it should stay completely isolated to wallet if implemented at all.

This seems like a nice feature for some users and I’m generally for it, but I’m also okay waiting on it until we get further down the road with node separation.

Any chance you can just cherrypick jamesob/bitcoin@92b620f https://github.com/jamesob/bitcoin/commit/92b620f26127b93f2b6fd389516f5e7b7d09ac7e so that the commit attribution remains intact? You can then do git commit –amend to make your changes.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/12801#issuecomment-379940642, or mute the thread https://github.com/notifications/unsubscribe-auth/AAL8uRzkqiRopFt184UWXGNEDTaK2ry3ks5tnAUtgaJpZM4S9fis .

ryanofsky commented at 12:21 pm on April 10, 2018: member

I wonder if it the concerns about code complexity would be addressed by splitting up the general purpose confirmation tracking code from the application-specific wallet notification logic. If you pulled the notify_command_template stuff out of WalletTransactionNotifier class, you could be left with a more general purpose class like:

 0class ConfirmationTracker
 1{
 2public:
 3   //! Create new confirmation tracker which invokes callbacks after a
 4   //! specified number of confirmations.
 5   ConfirmationTracker(int num_confirmations);
 6
 7   //! Register callback which will be called later when block at block_height
 8   //! has the specified number of confirmations, or immediately if it already
 9   //! has enough confirmations.
10   void addCallback(int block_height, function<void()> callback);
11
12   //! Handle new block being connected at block_height by invoking callbacks
13   //! registered at (block_height - num_confirmations), and dropping callbacks
14   //! previously registered at block_height (in case of reorgs).
15   void blockConnected(int block_height);
16private:
17   ...
18}

This could be a standalone class which could just go in libbitcoin_util and not be tied to the wallet. This way wallet notification code would not be much more complicated than it is now, and it would also be possible to imagine extending this class and using it more places like in the GUI, zmq code, or non-wallet RPCs.

skeees commented at 1:37 pm on April 10, 2018: contributor

That sounds like a great idea - and probably the right place for this to live to be more generally useful. If I see no objections to the principle of the entire thing, I’ll refactor along those lines and update in the coming days

ryanofsky commented at 2:03 pm on April 10, 2018: member

On the GetDepthInMainChain topic from #12801 (comment) and #12801 (comment), I think a more direct approach to tracking transaction confirmations inside the wallet might just be to add a new field to CMerkleTx. I described an idea for this here: #10973 (comment)

MarcoFalke added the label Needs rebase on Jun 6, 2018

luke-jr commented at 5:27 pm on June 12, 2018: member

I concur with the concept NACKs.

skeees commented at 6:45 pm on June 19, 2018: contributor

Enough NACKs that I’m going to close this. For those who have nacked (@luke-jr @jnewbery ) - was this a matter of the way it is implemented or more that this feature is completely undesired in the reference wallet implementation (fwiw, it seems like it is not infrequently requested) - if the latter, I’m happy to refactor implementation and open a new pr.

skeees closed this on Jun 19, 2018

jnewbery commented at 8:30 pm on June 19, 2018: member

was this a matter of the way it is implemented or more that this feature is completely undesired in the reference wallet implementation

I didn’t review the implementation. I was just NACKish on the concept.

promag commented at 8:40 pm on June 19, 2018: member

IMO we should avoid adding features to the current wallet if the plan is to separate from the node + multiprocess.

Re #12801 (comment)

Or did you have a different workaround in mind? @ryanofsky a RPC client can track confirmations without calling listtransactions.

laanwj removed the label Needs rebase on Oct 24, 2019

DrahtBot locked this on Dec 16, 2021

Add option to only notify after wallet transactions are confirmed #12801