Repository navigation
multi: follow-ups to option_independent_secrets and splice reserves - #15
Merged
Merged
Conversation
…ect to BOLT 7 says a node that receives a node_announcement with an unknown even feature bit SHOULD NOT connect to that node, and lightning-blake2b/bolts#17 has the even bit of option_independent_secrets count as one there. Pathfinding already refuses such nodes through feature.ValidateRoutable, but the graph bootstrapper and the autopilot agent still picked them as peers to dial and to open channels with. autopilot.Node now carries the node's announced features, and autopilot.Connectable checks them with ValidateRoutable. The graph bootstrapper skips nodes that fail it, and the agent leaves them out of the candidates it hands its heuristic. A node whose features aren't known is picked as before. Explicit connections (lncli connect, addpeer) and reconnections to channel peers are unchanged.
…aths findBlindedPaths picked intermediate nodes by RouteBlindingOptional alone, so an invoice or offer could hand out a blinded path through a node that announces an even feature bit we don't know, or the even bit of option_independent_secrets, which lightning-blake2b/bolts#17 has nodes treat as one. Pathfinding refuses to route through such a node since feature.ValidateRoutable, and a payer that doesn't know the bit refuses the path too. The onion message route search had the same gap, and its first hop is a node we may connect to on our own. Both now skip a node whose announced features fail ValidateRoutable, as pathfinding does. Our own node and channels the caller chains in are not checked, as before.
The comment quoted the old example of option_independent_secrets, a key derivation function over a private seed and the commitment number. lightning-blake2b/bolts#17 now names a value unique to the channel as a third input. The producer meets that already: its root is derived per channel, from a fresh revocation root key and the channel's own funding key, so it stands for both the seed and the channel's value. Say so. No change in behaviour.
The test slept 100ms after handing the link a tx_abort, betting that the link had handled it before the reserves were read. The read is a request on another queue than the peer's messages, so a slow run could read the reserves before the abort and pass whatever the abort did to them. It now waits for the splice handler to have the tx_abort. The link hands it over from its own goroutine, so from then on a request queued runs after the abort's handling: with the old behaviour, applying the splice reserves at the tx_abort, every run fails.
…kends Built without the dev tag, the interface tests sign the funding inputs they make with chanfunding's WalletAssembler, through input.SoleSignerSigHash, so with SIGHASH_ALL|SIGHASH_UNIFIED. The miner and the backends are a stock SHA256d regtest (btcd's regtest has no Blake2bHeight, and bitcoind runs without a BLAKE2b activation), which doesn't know the bit: bitcoind refused the funding transaction with "non mandatory script verify flag", and with btcd and neutrino it never reached the mempool, so single_funding_workflow waited for it to confirm until the test timed out. Tests whose transactions the wallet library signs passed, since its hash types stay SIGHASH_ALL unless the daemon changes them. The harness now opts out, as the daemon does in a development build, which is how make unit already ran these tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-ups from the reviews of #7, #10 and #13, and a test fix.
node_announcementas unknown). The graph bootstrapper and autopilot skip nodes whose announced featuresfeature.ValidateRoutablerefuses.lncli connect, persistent peers and backups are unchanged, and a node without such bits behaves as before.IndependentProducer's comment uses the spec's current terms (its root is already per channel).TestLinkTxAbortKeepsReserveswaits for the splice handler instead of sleeping; with the old link code it fails 20 of 20.Each change has a test that fails without it.