Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chainntnfs: validate conf/spend ntfn registration parameters #3405

Merged
merged 9 commits into from Aug 26, 2019

Conversation

@wpaulino
Copy link
Collaborator

commented Aug 15, 2019

A height hint not being set would cause lnd to scan for the confirmation/spend of a txid/outpoint/address from genesis.

The number of confirmations not being set within a confirmation request would cause the internal TxNotifier to deadlock when dispatching updates. To prevent this, we default to dispatching a notification upon one confirmation.

Fixes #3398.

@wpaulino wpaulino added this to the 0.8.0 milestone Aug 15, 2019
@wpaulino wpaulino requested a review from halseth as a code owner Aug 15, 2019
@wpaulino wpaulino requested review from cfromknecht and joostjager and removed request for halseth Aug 15, 2019
lnrpc/chainrpc/chainnotifer_server.go Outdated Show resolved Hide resolved
lnrpc/chainrpc/chainnotifer_server.go Outdated Show resolved Hide resolved
lnrpc/chainrpc/chainnotifer_server.go Outdated Show resolved Hide resolved
chainntnfs/txnotifier.go Show resolved Hide resolved
@wpaulino wpaulino force-pushed the wpaulino:chainrpc-sane-defaults branch from fb17176 to 89cd6a8 Aug 16, 2019
@wpaulino wpaulino changed the title chainrpc: default to one confirmation and require height hints chainntnfs: validate conf/spend ntfn registration parameters Aug 16, 2019
@wpaulino wpaulino force-pushed the wpaulino:chainrpc-sane-defaults branch from 89cd6a8 to 10af31e Aug 17, 2019
chainntnfs/txnotifier.go Outdated Show resolved Hide resolved
@wpaulino

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 19, 2019

As a result of having the validation checks being done at the chain notifier level, a bug was discovered with the BumpFee RPC that would cause us to scan the chain from genesis due to a 0 height hint. See the relevant commit for more details.

@wpaulino wpaulino requested a review from joostjager Aug 19, 2019
Copy link
Collaborator

left a comment

LGTM 🥓

@wpaulino wpaulino force-pushed the wpaulino:chainrpc-sane-defaults branch from 1556a7c to becd3d2 Aug 22, 2019
@wpaulino wpaulino requested a review from Roasbeef as a code owner Aug 22, 2019
@wpaulino wpaulino requested review from cfromknecht and joostjager and removed request for Roasbeef Aug 22, 2019
@wpaulino wpaulino force-pushed the wpaulino:chainrpc-sane-defaults branch from becd3d2 to 6244950 Aug 22, 2019
Copy link
Collaborator

left a comment

This pr did get quite out of hand. I didn't expect the move of subscription logic to txNotifier to involve so many changed lines of code. To me the end result looks definitely like an improvement. I hope you found it worthwhile to do. I left some non-blocking comments.

chainntnfs/txnotifier.go Outdated Show resolved Hide resolved
chainntnfs/bitcoindnotify/bitcoind.go Show resolved Hide resolved
chainntnfs/bitcoindnotify/bitcoind.go Show resolved Hide resolved
chainntnfs/btcdnotify/btcd.go Show resolved Hide resolved
@@ -690,9 +677,12 @@ func (n *NeutrinoNotifier) RegisterSpendNtfn(outpoint *wire.OutPoint,
//
// We'll update our filter first to ensure we can immediately detect the
// spend at tip.
if outpoint == nil {
outpoint = &chainntnfs.ZeroOutPoint
}

This comment has been minimized.

Copy link
@joostjager

joostjager Aug 22, 2019

Collaborator

Do this as first thing in this function, so that txNotifier.RegisterSpend can accept it as a value parameter?

chainntnfs/txnotifier.go Outdated Show resolved Hide resolved
lnwallet/btcwallet/signer.go Show resolved Hide resolved
err)
}
confs := int64(0)
if txDetail.Block.Height != -1 {

This comment has been minimized.

Copy link
@joostjager

joostjager Aug 22, 2019

Collaborator

-1 means unconfirmed?

This comment has been minimized.

Copy link
@wpaulino

wpaulino Aug 22, 2019

Author Collaborator

Yeah that's what's returned by the underlying call for unconfirmed transactions.

This comment has been minimized.

Copy link
@joostjager

joostjager Aug 23, 2019

Collaborator

const unconfirmed = -1 would be nice

sweep/walletsweep.go Show resolved Hide resolved
lnrpc/walletrpc/walletkit_server.go Outdated Show resolved Hide resolved
wpaulino added 5 commits Aug 17, 2019
…tionsNtfn
A height hint not being set would cause lnd to scan for the
confirmation/spend of a txid/outpoint/address from genesis.

The number of confirmations not being set within a confirmation request
would cause the internal TxNotifier to deadlock when dispatching
updates.
This prevents a deadlock while tearing down the TxNotifier if it's
currently blocked delivering a notification. By closing the quit chan
first, we ensure blocked sends/reads can exit and allow the TxNotifier
to proceed tearing down.
These fields are only relevant for spent transaction outputs.
wpaulino added 4 commits Aug 19, 2019
The cache wasn't really serving a purpose as FetchInputInfo isn't known
to be a hot path. Also, with a planned addition of returning the
confirmation status of an output within FetchInputInfo in a later
commit, caching won't be useful as we'll have to go to disk anyway to
determine the confirmation status.
We already have all of the information required for the outputs from the
ListUnspent method.
…utxos
In this commit, we address an issue that would cause us to scan from the
genesis block for the spend of an output that we wish to use to raise
the fee of a transaction through CPFP. This was due to setting a 0
height hint when constructing the input required by the sweeper and was
discovered due to the recently added validation checks at the chain
notifier level. We'll now use the current height as the height hint
instead as the sweeper will end up creating a new transaction that
spends the input.
@wpaulino wpaulino force-pushed the wpaulino:chainrpc-sane-defaults branch from 6244950 to 2cc5891 Aug 22, 2019
@wpaulino wpaulino requested a review from joostjager Aug 22, 2019
Copy link
Collaborator

left a comment

LGTM 👌 nice to see chainntnfs getting some more love!

@Roasbeef Roasbeef merged commit 3868bdc into lightningnetwork:master Aug 26, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.07%) to 61.1%
Details
@wpaulino wpaulino deleted the wpaulino:chainrpc-sane-defaults branch Aug 26, 2019
@alexbosworth

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

I see an issue here where I get this error when trying to restore an SCB

2 UNKNOWN: unable to unpack chan backup: a height hint greater than 0 must be provided

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.