Remove txindex requirement for full nodes #751

wpaulino · 2018-02-10T01:46:41Z

Fixes #527.

So far, lnd has required its backend btcd and bitcoind full nodes to enable the transaction index. It is used to retrieve details for transaction confirmations and/or already spent transactions. This PR aims to remove that requirement by introducing some fallback methods, in the event that the transaction index is not enabled.

When attempting to retrieve these details, we now have the following workflow:

We attempt to look up transactions using the transaction index.
If the transaction index is not enabled, we attempt to look up transactions by manually scanning the chain's blocks starting from the earliest height where the transaction could have been included in, to the current height.
3. In the case of pruned nodes, some of these blocks that we tried to scan above might have been missing because the node discarded them. Therefore, we decide to fetch these blocks from the node's peers and scan them.

~~I've also added some unit tests to make sure each part of the workflow above works as expected.~~

~~It's important to get this right, so it would be really helpful if people could spin up this patch locally on one of their test nodes and see how things go.~~

~~This depends on Roasbeef/btcwallet#14, so you'll either need to pull that locally as well or make the changes yourself within the vendor directory (very minor).~~

I realize there's a lot of code duplication, it might be worth looking into how to refactor some of these things. That's probably out of scope for this PR though.

wpaulino · 2018-02-12T06:11:12Z

Looks like to fully support pruned nodes, we'll also have to make some changes in the routing/chainview and lnwallet/btcwallet packages.

@Roasbeef, should I open a new PR to address this?

halseth

Excellent work on this one! I have reviewed the bitcoind part of this PR, and things are looking very good! 👍

My biggest code is that the massive code duplication that is being added to these files will make them very hard to maintain going forward. With the current way the notifier structs are set up, I can understand it is hard to extract out common code, but I think this PR would be an excellent opportunity to figure out how to refactor this. One way to go about this would be to use this PR to figure out a good way to do the refactoring, then push the refactoring as a separate PR (that does not change behaviour, only pure refactor), before rebasing this PR on top.

halseth · 2018-02-15T08:41:59Z

chainntnfs/bitcoindnotify/bitcoind.go

+
+	// netToParams maps the Bitcoin network specific magic number to its
+	// parameters.
+	netToParams = map[wire.BitcoinNet]*chaincfg.Params{


Slightly easier would be to store the params used when creating the notifier in the BitcoindNotifier struct.

halseth · 2018-02-15T08:44:40Z

chainntnfs/bitcoindnotify/bitcoind.go

+				txOut, err := b.chainConn.GetTxOut(&op.Hash,
+					op.Index, true)
+				if err != nil {
+					chainntnfs.Log.Error(err)


Do you want to continue here?

halseth · 2018-02-15T08:49:29Z

chainntnfs/bitcoindnotify/bitcoind.go

+
+// historicalSpendDetails retrieves the spend details for an already spent
+// outpoint and dispatches its spend notification.
+func (b *BitcoindNotifier) historicalSpendDetails(outpoint *wire.OutPoint,


Naming suggestion: notifyHistoricalSpendDetails to indicate this will notify clients.

halseth · 2018-02-15T08:50:06Z

chainntnfs/bitcoindnotify/bitcoind.go

+					err = b.historicalSpendDetails(&op,
+						msg.heightHint, bestHeight)
+					if err != nil {
+						chainntnfs.Log.Error(err)


continue?

halseth · 2018-02-15T08:55:51Z

chainntnfs/btcdnotify/btcd.go

-						close(ntfn.spendChan)
-					}
-					delete(b.spendNotifications, prevOut)
+					b.dispatchSpendNotification(prevOut, spendDetails)


Here you should either pass the clients instead of the outpoint, or remove the if _, ok := b.spendNotifications[prevOut]; ok { check above.

halseth · 2018-02-15T10:12:01Z

chainntnfs/btcdnotify/btcd.go

@@ -3,20 +3,24 @@ package btcdnotify
 import (
 	"errors"
 	"fmt"
+	"net"


Since this commit is getting quite big, maybe an idea to split it into two? :)

Not gonna review this file for now, as I reckon it's mostly the same changes as for bitcoind. We should really try to dedup some of the code, if possible.

halseth · 2018-02-15T10:12:08Z

chainntnfs/btcdnotify/btcd.go

+
+	// netToParams maps the Bitcoin network specific magic number to its
+	// parameters.
+	netToParams = map[wire.BitcoinNet]*chaincfg.Params{


halseth · 2018-02-15T10:14:57Z

chainntnfs/btcdnotify/btcd.go

+		// Begin looking for the spending transaction by fetching blocks
+		// from our peer at every height and scanning them.
+		for height := lastHeight; height <= bestHeight; height++ {
+			blockHash, err := b.chainConn.GetBlockHash(int64(height))


Not sure how easy it will be, but could we extract common code by putting the b.chainConn behind an interface that could be used by both btcd and `bitcoind``

halseth · 2018-02-15T10:18:39Z

chainntnfs/btcdnotify/btcd_test.go

+
+// testConfDetailsFromTxIndex tests that we can find the transaction from the
+// node's transaction index and that the details retrieved are correct.
+func testConfDetailsFromTxIndex(t *testing.T, miner *rpctest.Harness,


Are these similar to those for bitcoind? Possible to put the two notifiers behind a common interface, and reuse the tests?

halseth · 2018-02-15T10:19:14Z

chainntnfs/interface_test.go

 	"github.com/roasbeef/btcd/btcec"
+	"github.com/roasbeef/btcd/btcjson"


why was this file changed?

wpaulino · 2018-02-26T21:38:44Z

Thanks for the review @halseth!

This PR has been modified to provide manually scanning the backend's chain as the only fallback method when the transaction index is not enabled.

Before attempting to include support for pruned nodes, the chainntnfs package should be heavily refactored to avoid more code duplication.

Roasbeef

Nice work!

Before doing any more work to implement a pruned mode, or the like, think we'll need to aggressively refactor this package as a whole, in order to get rid of all the duplicated code across the various backend implementations.

Also I commented in-line within the PR w.r.t a possible danger when attempting to dispatch confirmations due to an incorrect height hint being passed by the caller. This PR mostly read swell to me, but we'll need to ensure that we're not introducing new bugs for nodes that are running without the txindex.

Roasbeef · 2018-03-21T23:02:26Z

chainntnfs/bitcoindnotify/bitcoind.go

 				if err != nil {
 					chainntnfs.Log.Error(err)
 				}
-				b.heightMtx.RLock()
+


Why is the mutex around access to the txConfNotifier no longer needed?

The mutex is only being used to guard the ChainNotifier's bestHeight. Calling txConfNotifier.Register without it should be fine since it doesn't have an effect on the bestHeight. I'm guessing it was needed at some point and it didn't get removed. I could be wrong though.

Roasbeef · 2018-03-21T23:08:18Z

chainntnfs/bitcoindnotify/bitcoind.go

 ) (*chainntnfs.TxConfirmation, error) {

-	// If the transaction already has some or all of the confirmations,
-	// then we may be able to dispatch it immediately.
-	// TODO: fall back to scanning blocks if txindex isn't on.
 	tx, err := b.chainConn.GetRawTransactionVerbose(txid)


The prior comment, minus the TOOD which will now be fixed, can likely remain.

Roasbeef · 2018-03-21T23:09:01Z

chainntnfs/bitcoindnotify/bitcoind.go

 	}

 	// As we need to fully populate the returned TxConfirmation struct,
 	// grab the block in which the transaction was confirmed so we can
 	// locate its exact index within the block.
 	blockHash, err := chainhash.NewHashFromStr(tx.BlockHash)
 	if err != nil {
-		return nil, fmt.Errorf("unable to get block hash %v for historical "+
-			"dispatch: %v", tx.BlockHash, err)
+		return nil, err


Why was the wrapped error (which had more detail) removed?

Roasbeef · 2018-03-21T23:09:24Z

chainntnfs/bitcoindnotify/bitcoind.go

 		if txHash == targetTxidStr {
-			txIndex = i
-			break
+			return &chainntnfs.TxConfirmation{


👍 on the simplification here

Roasbeef · 2018-03-21T23:13:15Z

chainntnfs/bitcoindnotify/bitcoind.go

+// earliest height the transaction could have been included in, to the current
+// height in the chain. If it is found, the confirmation details are returned.
+// Otherwise, an error is not returned.
+func (b *BitcoindNotifier) confDetailsManually(txid *chainhash.Hash,


With this change, we'll need to do a clean sweep through the entire codebase to ensure that heightHint is set properly. There are some known cases with neutrino (particularly in the contractcourt) where an incorrect height hint value can cause us to actually miss a notification all together. We'll need to extensively test this patch to ensure these holes are patched up.

I was running this patch on testnet for a bit and didn't run into any issues. I'll continue testing and see if I can run into any more.

Roasbeef · 2018-03-21T23:17:48Z

chainntnfs/bitcoindnotify/bitcoind.go

 				return nil, err
 			}
+		} else {
+			// Otherwise, we'll rely on manually scanning our chain


Why's this block necessary? Instead we can just query for the block hash of the height hint. Then the block below can be simplified to simply call into Rescan with the proper block hash.

Good point, will fix.

Roasbeef · 2018-03-21T23:19:24Z

chainntnfs/btcdnotify/btcd.go

 				return nil, err
 			}
+
+			targetTxidStr := outpoint.Hash.String()
+		out:


Similar comment here w.r.t this block not being necessary.

wpaulino · 2018-03-23T16:03:40Z

chainntnfs/bitcoindnotify/bitcoind.go

+	// First, we'll attempt to retrieve the transaction details using the
+	// backend node's transaction index.
+	txConf, err := b.confDetailsFromTxIndex(txid)
+	if err != nil {


Noticed this while addressing your comments: maybe we should proceed to use the heightHint if this fails, rather than returning the error?

With the new code, the nature of the error is likely critical. We'll need to properly test with both bitcoind and btcd to ensure that our logic catches the "no tx index" error properly.

Roasbeef · 2018-04-06T04:00:20Z

Can you rebase this to master? Thanks!

wpaulino · 2018-04-06T06:28:33Z

Rebased.

Roasbeef

Needs YAR (yet-another-rebase) due to some changes that recently landed to make mempool spends, but afterwards

LGTM ⛄️

Roasbeef · 2018-04-10T04:23:31Z

chainntnfs/btcdnotify/btcd.go

+				"with height %d", height)
+		}
+
+		block, err := b.chainConn.GetBlockVerbose(blockHash)


Worth adding a TODO here that we can actually just fetch the neutrino filters here. Will save us a bit of processing as we can quickly ascertain if a transaction maybe includes a block or not.

Roasbeef · 2018-04-10T04:31:38Z

When we (eventually) port over the integration tests to run on all backends, we should ensure that we're able to start and properly execute the tests against instances of btcd and bitcoind that don't have the txindex active.

halseth

This change looks good to me now, only a nit about a confusing comment 💯

The backend logic is becoming very similar for btcd and bitcoind now (which is good), maybe add a TODO to extract common code?

halseth · 2018-04-11T09:15:03Z

chainntnfs/bitcoindnotify/bitcoind.go

+
+// confDetailsFromTxIndex looks up whether a transaction is already included
+// in a block in the active chain by using the backend node's transaction index.
+// If it is found, the confirmation details are returned. Otherwise, an error


Comment describing if/when an error is returned is confusing.

halseth · 2018-04-11T09:16:54Z

chainntnfs/bitcoindnotify/bitcoind.go

-		return nil, fmt.Errorf("unable to query for txid(%v): %v", txid, err)
+	}
+
+	// Make sure we actually retrieved a transaction that is included in a


thumbsup on this refactoring!

halseth · 2018-04-11T09:19:17Z

chainntnfs/bitcoindnotify/bitcoind.go

+// block in the active chain by scanning the chain's blocks, starting from the
+// earliest height the transaction could have been included in, to the current
+// height in the chain. If it is found, the confirmation details are returned.
+// Otherwise, an error is not returned.


Same. I think I understand what this means, but it's a bit confusing 😛

wpaulino · 2018-04-16T17:10:25Z

Rebased and added TODOs.

halseth · 2018-04-17T11:29:43Z

chainntnfs/bitcoindnotify/bitcoind.go

+// confDetailsFromTxIndex looks up whether a transaction is already included
+// in a block in the active chain by using the backend node's transaction index.
+// If the transaction is found, its confirmation details are returned.
+// Otherwise, a nil error is returned. A non-nil error may also be returned if


Suggestion:
"If the transaction is found in the index, its confirmation details are returned. If it is not found in the index, nil is returned."
I think that should make it more clear that a non-nil error is critical(?).

Before this commit, we relied on the need of full nodes to enable the transaction index. This allowed us to fetch historical details about transactions in order to register and dispatch confirmation and spend notifications. This commit allows us to drop that requirement by providing a fallback method to use when the transaction index is not enabled. This fallback method relies on manually scanning blocks for the transactions requested, starting from the earliest height the transactions could have been included in, to the current height in the chain.

Now that we have a fallback method for when the transaction index is not enabled, its requirement to be enabled for a full node is no longer needed, so we can safely remove this check.

Roasbeef

LGTM 🔥

Roasbeef · 2018-04-18T01:20:05Z

Will make an issue for a clean sweep to ensure height hints are always properly set. Last I investigated, there were some areas in the utxo nursery, and also the contractcourt where an incorrect height hint could at times lead to failure to actually sweep an output or respond to an event.

See lightningnetwork#751

wpaulino force-pushed the txindex branch from 00ba370 to 681d30a Compare February 10, 2018 15:16

meshcollider added funding Related to the opening of new channels with funding transactions on the blockchain channels backend Related to the node backend software/interface (e.g. btcd, bitcoin-core) labels Feb 11, 2018

wpaulino force-pushed the txindex branch from 681d30a to 2a5f857 Compare February 14, 2018 17:27

halseth suggested changes Feb 15, 2018

View reviewed changes

wpaulino force-pushed the txindex branch from 2a5f857 to bc5889c Compare February 26, 2018 21:38

Roasbeef requested changes Mar 21, 2018

View reviewed changes

wpaulino force-pushed the txindex branch from bc5889c to 04126d8 Compare March 23, 2018 15:59

wpaulino commented Mar 23, 2018

View reviewed changes

wpaulino force-pushed the txindex branch 2 times, most recently from 16244be to 7e7c5ef Compare April 6, 2018 06:18

Roasbeef mentioned this pull request Apr 7, 2018

Retrieving genesis coinbase tx not allowed #1031

Closed

Roasbeef previously approved these changes Apr 10, 2018

View reviewed changes

halseth previously approved these changes Apr 11, 2018

View reviewed changes

wpaulino dismissed stale reviews from halseth and Roasbeef via d8169f1 April 16, 2018 15:37

wpaulino force-pushed the txindex branch from 7e7c5ef to d8169f1 Compare April 16, 2018 15:37

halseth reviewed Apr 17, 2018

View reviewed changes

wpaulino added 2 commits April 17, 2018 14:59

chainregistry: remove txindex requirement for btcd and bitcoind

ead9555

Now that we have a fallback method for when the transaction index is not enabled, its requirement to be enabled for a full node is no longer needed, so we can safely remove this check.

wpaulino force-pushed the txindex branch from 97d96f8 to ead9555 Compare April 17, 2018 19:38

Roasbeef approved these changes Apr 18, 2018

View reviewed changes

Roasbeef merged commit 1f4a02e into lightningnetwork:master Apr 18, 2018

wpaulino deleted the txindex branch April 18, 2018 03:18

dbolser added a commit to dbolser/lnd that referenced this pull request Apr 28, 2018

Updating in line with a PR

37cc62e

See lightningnetwork#751

This was referenced Apr 28, 2018

Updating in line with a PR #1148

Merged

Guide assumes testnet, adding a note about mainnet #1154

Closed

LightningK0ala mentioned this pull request Jun 18, 2018

Specify --txindex optional for bitcoind in installation docs #1413

Closed

rootzoll mentioned this pull request Aug 30, 2018

Remove txindex from bitcoin config raspiblitz/raspiblitz#69

Closed

ghost mentioned this pull request Sep 12, 2018

Update README with bitcoind setup lightninglabs/lightning-app#664

Merged

tutacrypto mentioned this pull request Feb 16, 2019

bitcoind has -prune=0 hardcoded but the guide says it's optional lightning-power-users/node-launcher#182

Open

3 tasks

		"github.com/roasbeef/btcd/btcec"
		"github.com/roasbeef/btcd/btcjson"

Remove txindex requirement for full nodes #751

Remove txindex requirement for full nodes #751

Conversation

wpaulino commented Feb 10, 2018 • edited Loading

wpaulino commented Feb 12, 2018 • edited Loading

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

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

wpaulino commented Feb 26, 2018

Roasbeef 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

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 Apr 6, 2018

wpaulino commented Apr 6, 2018

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef commented Apr 10, 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

Choose a reason for hiding this comment

wpaulino commented Apr 16, 2018

halseth Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

Roasbeef left a comment

Choose a reason for hiding this comment

Roasbeef commented Apr 18, 2018

wpaulino commented Feb 10, 2018 •

edited

Loading

wpaulino commented Feb 12, 2018 •

edited

Loading

halseth Apr 17, 2018 •

edited

Loading