chainntnfs: ensure proper fallback to scanning tx manually #1585

wpaulino · 2018-07-18T21:07:06Z

In this commit, we address a bug where it's possible that we still
attempt to manually scan for a transaction to determine whether it's
been included in the chain even after successfully checking the txindex
and not finding it there. Now, we'll short-circuit this process by
exiting early if the txindex lookup was successful but the transaction
in question was not found. Otherwise, we'll fall back to the manual
scan.

Roasbeef

Nice catch! The code reads well to me. However, ideally we also have a test that fails as is in the current master. See my comment in line which goes over how to write such a test.

Roasbeef · 2018-07-18T22:43:49Z

chainntnfs/bitcoindnotify/bitcoind.go

+		// scanning the chain manually. We'll also need to look at the
+		// error message returned as the error code is used for multiple
+		// errors.
+		txNotFoundErr := "No such mempool or blockchain transaction"


Have we confirmed that this is the exact help text? Ideally we also have a test along with this PR which will fail if the backend detects a query to the GetBlocks when we query for a transaction that was never even broadcast. The test would need to skip the neutrino backend, and only execute for the btcd and bitocind backends.

wpaulino · 2018-07-24T01:13:48Z

Updated the PR to include some tests. I realize there's a lot of code duplication, but I have a branch locally where I've begun to refactor all the different ChainNotifier implementations into one. Then we can refactor our tests to include a set of white-box and black-box tests, like we do with invoices in zpay32.

halseth · 2018-07-24T08:41:33Z

chainntnfs/bitcoindnotify/bitcoind.go

+	// We'll then check the status of the transaction lookup returned to
+	// determine whether we should proceed with any fallback methods.
+	switch {
+	// The transaction was found within the node's txindex.


comment should say "mempool"

halseth · 2018-07-24T08:42:35Z

chainntnfs/bitcoindnotify/bitcoind.go

-	if txConf != nil {
-		return txConf, nil
+	// The transaction was not found within the node's mempool or txindex.
+	case txStatus == txNotFoundIndex && err == nil:


Should we return in the case of err != nil?

This indicates that the transaction simply doesn't exist, as it was not found within the txindex or mempool. The err != nil case is handled in the default case, as that means something went wrong when looking at the index (either because it's disabled or something else went wrong), so we should fall back to scanning manually.

I think we should check the error first, as now it can return (_, 0, err) which will match the txFoundMempool case.

halseth · 2018-07-24T08:45:55Z

chainntnfs/bitcoindnotify/bitcoind.go

 	}

 	// Make sure we actually retrieved a transaction that is included in a
 	// block. Without this, we won't be able to retrieve its confirmation
 	// details.
 	if tx == nil || tx.BlockHash == "" {
-		return nil, nil
+		return nil, txFoundMempool, nil


Is it in the mempool in this case? Update comment to explain.

Doesn't the comment as is imply this though by it not being included in a block?

If tx == nil, does that mean it is in the mempool?

halseth · 2018-07-24T08:48:29Z

chainntnfs/bitcoindnotify/bitcoind_test.go

+	if err := waitForMempoolTx(miner, txid); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := miner.Node.Generate(1); err != nil {


Check the mempool case before generating a block?

Can't do so for bitcoind at the moment until btcsuite/btcd#1222 is in as we'll need to wait for bitcoind to detect the transaction in its mempool.

halseth · 2018-07-24T08:50:56Z

chainntnfs/btcdnotify/btcd.go

+// txConfStatus denotes the status of a transaction's lookup.
+type txConfStatus uint8
+
+const (


Note: as discussed, would be nice to later make these shared between all backends, and then include them in SpendEvents etc

halseth · 2018-07-24T08:52:03Z

chainntnfs/btcdnotify/btcd.go

+	// We'll then check the status of the transaction lookup returned to
+	// determine whether we should proceed with any fallback methods.
+	switch {
+	// The transaction was found within the node's txindex.


All the comments apply to btcd as well, I guess.

Roasbeef · 2018-07-31T01:13:51Z

Build failing on all instances. Looks to be an issue with the commit of btcd it's targeting?

wpaulino · 2018-07-31T01:21:20Z

It's due to the removal of extended filters in neutrino. This PR as is adds a lot of duplicate code for tests. After seeing how they've been handled in #1439, I'll be modifying this PR to closely follow that approach. Will ping once done.

Roasbeef · 2018-08-14T05:06:40Z

Ping on rebase, almost forgot about this one!

In this commit, we extract some of the helper test variables and functions into their own file and guard them under a build flag. This is needed as some unit tests will be introduced in a future commit where most of the same functions within the interface tests are reused. In order to prevent these variables and functions from being exportable, we guard them by the "debug" build tag.

In this commit, we address a bug where it's possible that we still attempt to manually scan for a transaction to determine whether it's been included in the chain even after successfully checking the txindex and not finding it there. Now, we'll short-circuit this process by exiting early if the txindex lookup was successful but the transaction in question was not found. Otherwise, we'll fall back to the manual scan.

halseth

This looks pretty good 👍

One thing I that could be improved is to document more clearly the relation between the different conf states. A suggestion would be to make say confDetailsFromTxIndex return states [mempool, block, not-found], and confDetailsManually return states [found, not-found]. Then historicalConfDetails could use these values to determine which of the txConfStatuss to return (and then I believe we could replace all *notFound statuses there with a single txNotFound status.

Also needs a rebase!

halseth · 2018-08-23T08:51:41Z

chainntnfs/bitcoindnotify/bitcoind.go

@@ -431,84 +457,102 @@ func (b *BitcoindNotifier) handleRelevantTx(tx chain.RelevantTx, bestHeight int3
 // historicalConfDetails looks up whether a transaction is already included in a
 // block in the active chain and, if so, returns details about the confirmation.
 func (b *BitcoindNotifier) historicalConfDetails(txid *chainhash.Hash,
-	heightHint, currentHeight uint32) (*chainntnfs.TxConfirmation, error) {
+	heightHint, currentHeight uint32) (*chainntnfs.TxConfirmation, txConfStatus, error) {


nit: long line?

halseth · 2018-08-23T08:54:16Z

chainntnfs/bitcoindnotify/bitcoind.go

-	if txConf != nil {
-		return txConf, nil
+	// The transaction was not found within the node's mempool or txindex.
+	case txStatus == txNotFoundIndex && err == nil:


I think we should check the error first, as now it can return (_, 0, err) which will match the txFoundMempool case.

halseth · 2018-08-23T08:56:36Z

chainntnfs/bitcoindnotify/bitcoind.go

 	}

 	// Make sure we actually retrieved a transaction that is included in a
 	// block. Without this, we won't be able to retrieve its confirmation
 	// details.
 	if tx == nil || tx.BlockHash == "" {
-		return nil, nil
+		return nil, txFoundMempool, nil


If tx == nil, does that mean it is in the mempool?

halseth · 2018-08-23T08:58:08Z

chainntnfs/bitcoindnotify/bitcoind_test.go

+	// Now, we'll create a test transaction, confirm it, and attempt to
+	// retrieve its confirmation details.
+	//
+	// TODO(wilmer): add mempool case once trickle timeout can be specified


this is now added.

Roasbeef requested changes Jul 18, 2018

View reviewed changes

wpaulino force-pushed the proper-txindex-fallback branch 2 times, most recently from 530f182 to 66439b3 Compare July 24, 2018 01:11

halseth reviewed Jul 24, 2018

View reviewed changes

Roasbeef added notifications P2 should be fixed if one has time needs testing PR hasn't yet been actively tested on testnet/mainnet first pass review done PR has had first pass of review, needs more tho labels Aug 14, 2018

Roasbeef added this to the 0.5 milestone Aug 14, 2018

wpaulino force-pushed the proper-txindex-fallback branch 2 times, most recently from a5acd1c to fd50eb0 Compare August 17, 2018 02:50

wpaulino added 6 commits August 16, 2018 20:39

build: update to btcd with rpctest.Harness w/o indexes

970f726

chainntnfs: guard test chain notifier under debug flag

150f706

multi: enable txindex on miner harness

4eda975

Makefile: point btcd to fork

a317236

wpaulino force-pushed the proper-txindex-fallback branch from 526b289 to a317236 Compare August 17, 2018 03:39

halseth added the needs rebase PR has merge conflicts label Aug 22, 2018

halseth suggested changes Aug 23, 2018

View reviewed changes

cfromknecht mentioned this pull request Aug 24, 2018

Txindex proper fallback #1778

Merged

Roasbeef closed this in #1778 Aug 24, 2018

wpaulino deleted the proper-txindex-fallback branch August 27, 2018 19:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chainntnfs: ensure proper fallback to scanning tx manually #1585

chainntnfs: ensure proper fallback to scanning tx manually #1585

wpaulino commented Jul 18, 2018

Roasbeef left a comment

Roasbeef Jul 18, 2018

wpaulino commented Jul 24, 2018

halseth Jul 24, 2018

wpaulino Aug 17, 2018

halseth Jul 24, 2018

wpaulino Aug 17, 2018

halseth Aug 23, 2018

halseth Jul 24, 2018

wpaulino Aug 17, 2018

halseth Aug 23, 2018

halseth Jul 24, 2018

wpaulino Aug 17, 2018

halseth Jul 24, 2018

halseth Jul 24, 2018

Roasbeef commented Jul 31, 2018

wpaulino commented Jul 31, 2018

Roasbeef commented Aug 14, 2018

halseth left a comment

halseth Aug 23, 2018

halseth Aug 23, 2018

halseth Aug 23, 2018

halseth Aug 23, 2018

chainntnfs: ensure proper fallback to scanning tx manually #1585

chainntnfs: ensure proper fallback to scanning tx manually #1585

Conversation

wpaulino commented Jul 18, 2018

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wpaulino commented Jul 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef commented Jul 31, 2018

wpaulino commented Jul 31, 2018

Roasbeef commented Aug 14, 2018

halseth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment