'confirmed' event not emitted for coinbase or any TX not already inserted into txdb

[pinheadmz]

This is an issue brought to my attention by @Maduranga on slack.

When generating blocks in regtest, the wallet does not emit a 'confirmed' event for the coinbase transaction (other events are emitted as expected: 'tx', 'balance', 'address', etc.)

Looking into it further, I notice that there are two paths in txdb where a transaction is confirmed. Transactions always start at txdb.add(). If the transaction already exists in txdb (with 0 confirmations), it is redirected to txdb.confirm():

https://github.com/bcoin-org/bcoin/blob/99638ac1090402b6efe3364e0de3be3be903626d/lib/wallet/txdb.js#L423-L435

txdb.confirm() DOES emit the 'confirmed' event.

However, if the TX has never before been seen by txdb (as is the case with coinbase transactions), the TX is directed to txdb.insert(). If txdb.insert() is passed a block, it does everything txdb.confirm() would have done -- EXCEPT emit the 'confirmed' event:

https://github.com/bcoin-org/bcoin/blob/99638ac1090402b6efe3364e0de3be3be903626d/lib/wallet/txdb.js#L582-L588

It will still emit the 'tx' event at the end of the function.

Question for the team: Should we emit a 'confirmed' event in that if(block) block? It would be emitted simultaneously with the 'tx' event, but is logically correct. Especially if the user is specifically listening for 'confirmed' events. As we've seen with other wallet issues involving the lookahead etc, we can not always expect a TX to exist in the txdb before the first confirmation.

[pinheadmz]

I need to double check this, but in resolving https://github.com/bcoin-org/bcoin/issues/887 I have also discovered that 'tx' events are not emitted for coinbase transactions, either.

[braydonf]

So I think there are a few options here:

1. Remove both confirmed and unconfirmed events. Have confirming a transaction that already exists in the database emit a tx event instead of a confirmed event. The confirmed count on the details will show it as being confirmed or unconfirmed. There would potentially be multiple tx events for a transaction, once when it was added as unconfirmed, another when it was confirmed, and potentially another if it where to ever become unconfirmed. For a transaction that was added and confirmed there would only be a single tx event (as is current).
2. Emit a confirmed event in addition to the tx event when a transaction is added to the database and confirmed at the same time. This will however result in the same transaction details being sent twice. A listener of the events, would then unnecessary handle both cases at the same time.
3. Keep it as-is and clarify that the confirmed event will not happen for every tx confirmation, and both the tx and confirmed events should be watched for confirmations. Though this is somewhat non-intuitive.

[braydonf]

We have related test for this now at https://github.com/bcoin-org/bcoin/blob/master/test/wallet-test.js#L2161-L2171

[pinheadmz]

I think option 1 actually makes the most sense, but I'm worried about breaking use cases already in place. I guess if it's documented in the chagelog we should be fine, those users can just switch to the tx event.

[martindale]

What's the latest on this issue? I ran into it in a recent project.

[pinheadmz]

@martindale do you have an opinion on the solutions Braydon enumerates?

I think the reason this issue didn't proceed any where is that most application built with bcoin listen to "tx" events for the most part, and then also listen to "block" events from the full node which are used in the application layer to count confirmations.

[martindale]

I feel like both tx and confirmed (at +100 blocks?) events make sense here, as the application layer shouldn't have issues with duplicate transactions (given the mutability fix) — but as I understand it in the original report, even the tx event is not being emitted here?

[pinheadmz]

Right now the way to think about confirmed is "this transaction that we already know about from when it was in the mempool, is now confirmed"

The tx event right now means "this is a new transaction we haven't seen before". If it is in a block, the details returned by the event will indicate that. (if it is only in the mempool, it will have properties height: -1 and block: null).

[martindale]

Should I expect to receive the tx event when the block is received, or at n + 100 blocks? Or at both?

For my case, the tx event is enough, and I wouldn't mind receiving it twice — so long as I receive it! :)

[pinheadmz]

From bcoin's perspective, the only confirmation that matters is the first confirmation. Developers and users are expected to use application logic to count additional confirmations after that (by listening for "block" events), otherwise it gets to be too much work and different applications may have different confirmation requirements, etc.

Due to this issue, you should listen for both tx and confirmed events -- they will occasionally be redundant but there are edge cases where one will fire and not the other.

[martindale]

We're not getting confirmed or tx events for coinbase transactions, though?

I need to double check this, but in resolving #887 I have also discovered that 'tx' events are not emitted for coinbase transactions, either.

[pinheadmz]

We're not getting confirmed or tx events for coinbase transactions, though?

I need to double check this, but in resolving #887 I have also discovered that 'tx' events are not emitted for coinbase transactions, either.

So I just did a quick check and caught tx but not confirmed events for a coinbase TX:

to reproduce:

• terminal 1: start bcoin in regtest
• terminal 2: bwallet-cli listen which will open sockets and output all wallet events
• terminal 3: bcoin-cli rpc generatetoaddress 1 'bwallet-cli rpc getnewaddress'

When I ran this just now I got events in this order:

1. tx
2. balance
3. address

[martindale]

Which class is expected to emit these events? We're not using HTTP interface, but rather instantiating Wallet and FullNode from within our application (sorry I haven't gotten back to #927 yet, but we discarded those changes after it was mentioned that events wouldn't be emitted).

[pinheadmz]

All wallet events are emitted by the WalletDB object. More here: https://bcoin.io/guides/events

[martindale]

All wallet events are emitted by the WalletDB object. More here: https://bcoin.io/guides/events

Okay, upon further digging into the code (there don't seem to be any docs for using bcoin as a library? The WalletDB class doc is devoid of method definitions, so browsing the code seems to be the only way to figure things out...) maybe we're not assigning a NodeClient to the database?

What's the canonical way of "subscribing" a WalletDB (or Account?) to events from a FullNode instance?

[pinheadmz]

Ah yeah our JSDocs are broken. That bcoin.io guide has lots of links to the code. The wallet events are also documented in the API docs: https://bcoin.io/api-docs/#wallet-sockets but we desperately need more community contributions to complete the documentation and help prevent issues like yours.

What's the canonical way of "subscribing" a WalletDB (or Account?) to events from a FullNode instance?

So I'm not entirely sure what you mean here. Let's take Purse.io architecture as an example. Each user is assigned an account from the BIP44 strutcure. The application layer uses WalletClient and NodeClient objects as described in the api docs and the events guide. That guide is actually a bit outdated now and the api docs are better:

const {WalletClient, NodeClient} = require('bcoin');

The application layer listens for BOTH tx and confirmed events, emitted from WalletClient. The event is accompanied by TXDetails which contains the tx, and the tx outputs include a datum called account if it belongs to the wallet. This is how Purse credits user's accounts in the application database. Both tx and confirmed events have the same shape and will have a non-null value for block and height if the tx was confirmed in a block (this is true for both event types).

Let's say as a platform we required 6 confirmations before crediting user accounts. The application just keeps track of all TXs with < 6 confirmations in memory or in a db table.

Meanwhile, the application ALSO listens for block events from NodeClient. When a block comes in, the application can increment the # of confirmations for all the transactions in that table. Once a TX is >= 6 confirmations, the tx can be removed from the table and the user account is credited.

If you're not using the HTTP interface, you can listen for the events directly:

FullNode.on('block', (block, ChainEntry) => {...})

In bcoin the wallet is a separate module and can either be run as a remote process or as a plugin, which is the default. If you are running with the wallet as a plugin, you have to dig out the walletDB if you want to listen for events directly from it:

const wdb = FullNode.plugins.walletdb.wdb;
wdb.on('tx', (walletID, tx, details) => {...});
wdb.on('confirmed', (walletID, tx, details) => {...});

Sorry if this looks messy, but bcoin "the canonical" method of accessing these events is with the client, which connects to the node and wallet via http sockets, and everything is a lot easier on that interface.

I recommend you try bwallet-cli listen and examine for yourself what events are emitted during a run of your application's work flow.

If you want more direct support you can chat with us on slack: https://bcoin.io/slack-signup.html or IRC #bcoin