BIP-68: Mempool-only sequence number constraint verification #6312
Conversation
|
Mind fixing that BIP link? 404 |
|
Fixed. On Sun, Jun 21, 2015 at 11:28 AM, Peter Todd notifications@github.com
|
|
Now again, could you please do up a quick demo example app that actually uses this? e.g. like my https://github.com/petertodd/checklocktimeverify-demos/blob/master/micropayment-channel.py Doesn't have to be much, but we really need a worked out example here... Notably, see how without a way to fix tx mutability the nSequence proposal really isn't any different in practice than just signing increasingly decreasing nLockTime's? Because if you could safely sign the transaction it'd already be mined and thus you don't need a relative nSequence - an absolute nLockTime calculated from the height/time of the tx you know already is in the chain is fine. IMO there's no reason to merge this until it has a safe use-case that provides a concrete benefit over the existing status quo; a simple change in what exact number is being signed isn't a concrete benefit. |
|
Why doesn' this patch include the code for OP_CHECKSEQUENCEVERIFY? |
|
@btcdrak that code is here: https://github.com/maaku/bitcoin/tree/checksequenceverify It is entirely separate from what this pull request accomplishes. For example, it is possible to do simple transaction replacement without CHECKSEQUENCEVERIFY, as explained in the BIP. Likewise the source code of the CSV branch does not in fact depend on this pull request -- all CSV does is verify a constraint on the sequence number of the input, it does not depend on that sequence number having consensus-enforced semantics. |
|
@maaku I think the value of this PR in isolation is limited. You really need the opcode as well. It would be wonderful if you could publish a separate PR+BIP for CSV if you dont wish to combine them. CSV is a very useful feature and one I think we clearly need to help address scalability. I see no profit in not getting it on the table now especially since it's been deployed in Elements already and is clearly working. |
|
I'm working on it. Things take time.
|
| @@ -14,5 +14,7 @@ static const unsigned int MAX_BLOCK_SIGOPS = MAX_BLOCK_SIZE/50; | |||
| static const int COINBASE_MATURITY = 100; | |||
| /** Threshold for nLockTime: below this value it is interpreted as block number, otherwise as UNIX timestamp. */ | |||
| static const unsigned int LOCKTIME_THRESHOLD = 500000000; // Tue Nov 5 00:53:20 1985 UTC | |||
| /** Threshold for inverted CTxIn::nSequence: below this value it is interpreted as a relative lock-time, otherwise ignored. */ | |||
| static const uint32_t SEQUENCE_THRESHOLD = (1 << 31); | |||
There was a problem hiding this comment.
Probably should call this something other than sequence?
|
needs rebase |
|
Rebased. |
| @@ -860,6 +942,18 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa | |||
| view.SetBackend(dummy); | |||
| } | |||
|
|
|||
| // Enforce sequnce numbers as relative lock-time | |||
|
concept ACK |
|
Refs #6564 |
|
Agreed with @petertodd that this is fairly pointless without BIP-62 (malleability), but I do want both for lightning. (I also note that we should in future detach the locktime field from requiring non-FFFFFFFF nSequence; they're completely orthogonal AFAICT. But that's a minor soft-fork.) |
|
@petertodd isn't the point of this that it is relative? That is, I don't need to know what block the first transaction is in, nor the follow up transactions? |
|
@rustyrussell @petertodd : what relative lock-time gives you is a committed response window to a proof-of-publication. With CHECKSEQUENCEVERIFY (#6564) there is indeed some features you can get that in some situations aren't just replicatable with CHECKLOCKTIMEVERIFY. However the most exciting use cases also require something additional still. In the case of lightning, not just BIP 62, but some sort of normative txid. @dcousens I believe @petertodd's point is that since all the inputs are already on the block chain, you can just set nLockTime to the confirmed age of the input + the relative lock-time. And if the inputs are not all confirmed, then you are building chains of transactions that aren't safe due to malleability. So really this is a feature that becomes most useful when we have a definitive fix for malleability, which we will in the not so distant future. |
|
Yup, my objection was a somewhat pedantic point of process; now that CSV has been proposed it's moot objection. Will review all this stuff soon FWIW, travelling right now... Per usual. :/ On 17 August 2015 18:43:33 GMT-07:00, Mark Friedenbach notifications@github.com wrote:
|
Right, thanks for clearing that up @maaku :) |
|
@maaku Just to clarify (though a bit OT): Lightning wants normalized txid for simpler dual-input anchors, but doesn't need it (the escape tx design seems to work). Lightning needs normalized txid for outsourcing of revocation enforcement. |
|
Fixed @luke-jr's nits. |
|
This change to BIP68 (bitcoin/bips#260) while making a clearer specification just broke completely this PR. Basically this PR assumed similar blocktime calculation (MTP or not) policy for both nLockTime and sequence lock time. But the change of specification makes MTP mandatory for sequence locktime while nLockTime is still depends on MTP activation. This PR, if preferred over alternatives, will need to be updated. (or the change to specification reverted) I expect the changes to tests code and logic to be significant though to be worth the trouble. I think the best choices are either to move to Morcos' PR (#7184) which is very different from this implementation. Or, reverting bitcoin/bips#260 if people wants to stick with this well reviewed PR. |
|
The assumption of MTP only was discussed at the IRC meeting and that BIP68 and BIP113 need to be softforked together. I am in favour of #7184. However, the assumption was if this PR is chosen over #7184 then this PR would be updated accordingly to the new specification. It's just #7184 already made the assumption. It's a lot cleaner to assume MTP and really, there is no situation where it would not be MTP because we are deploying 68 and 113 together. |
|
Yeah sorry about that, I guess it is a bit tough to try to keep BIP spec in line with code. One of the hesitations with switching to #7184 has been that this PR is already heavily reviewed and tested. I think this is wrong. It had a lot of preliminary review, but then the code changed, and I think its had only minimal testing. An example is the checking of sequence locks that is throughout the wallet and GUI code in this PR. I believe those would lead to unacceptable performance for any node with a half way decent size wallet. They would slow down a lot of operations and also make the utxo cache worthless. |
|
@morcos I absolutely agree, we cannot claim this PR is well reviewed because each reviewer, reviewed it in a different state. Regardless, this has been beneficial in refining the specification and implementation. |
|
Understood, I have no particular feeling about it, just worried #7184 would delay CSV adoption for months. |
It is true that the code changed after most of the reviews were done. Fortunately @btcdrak kept the commits separated so that concern has an easy solution: remove the latest changes. |
|
@jtimon one of my change is actually a bug fix... sadly it was squashed. Regardless of what happen, this should be kept. The problem was that the birth time of an unconfirmed coin was set to blockTime which depended on MTP activation. Whereas confirmed coins always used MTP time. |
|
I have to say it was bad form that #7184 merged everything into one commit as it makes the review significantly harder (and less critically, credit assignment less clear). I think it was also bad form to split off a separate PR for the exact same feature, rather than discuss the alternative implementation pathway in the same thread, but I digress... |
|
@NicolasDorier Well, if it was a bugfix then it's not sad that it was squashed, is it? In my opinion no optimization had been needed in the mining code this could have been merged before 0.12 with no backporting problems. If the optimizations hadn't been urgent they could have been later during 0.13.99. But now it's too late for that, thus the need for #7176 . All the possible fixes could be built on top of the same branch (this one, for example) to compare them more easily, but seems not everybody agrees that a common base makes branches easier because I have already say this without the different proponents agreeing on a common base, and @maaku seems to be the only other person that thinks #7187 should be based on this PR instead of #7184 so I shouldn't insist on this. |
|
By "sad being squashed", I mean that if you want to revert recent changes I made, at least you should keep the bug fix commit. Extracting it would have been simpler without squash. (but well, the bug is not a big deal to fix again) |
|
Which is why it is better not to squash until the very end before merge.
|
|
As already posted on #7184 maaku/bitcoin@sequencenumbers...morcos:7184onorig6312 Feel free to incorporate that here if that is preferred. |
|
Tested-by: Rusty Russell rusty@rustcorp.com.au (I merged both this and #6564 for the test, with only a trivial conflict to resolve). |
|
Closing in favour of #7184 |
SequenceLocks functions are used to evaluate sequence lock times or heights per BIP 68. The majority of this code is copied from maaku in bitcoin#6312 Further credit: btcdrak, sipa, NicolasDorier
SequenceLocks functions are used to evaluate sequence lock times or heights per BIP 68. The majority of this code is copied from maaku in bitcoin#6312 Further credit: btcdrak, sipa, NicolasDorier
SequenceLocks functions are used to evaluate sequence lock times or heights per BIP 68. The majority of this code is copied from maaku in bitcoin#6312 Further credit: btcdrak, sipa, NicolasDorier
SequenceLocks functions are used to evaluate sequence lock times or heights per BIP 68. The majority of this code is copied from maaku in bitcoin#6312 Further credit: btcdrak, sipa, NicolasDorier
SequenceLocks functions are used to evaluate sequence lock times or heights per BIP 68. The majority of this code is copied from maaku in bitcoin#6312 Further credit: btcdrak, sipa, NicolasDorier
SequenceLocks functions are used to evaluate sequence lock times or heights per BIP 68. The majority of this code is copied from maaku in bitcoin#6312 Further credit: btcdrak, sipa, NicolasDorier
Essentially enforces sequence numbers as a relative lock-time as an IsStandard() rule to make it easy to test applications using it; blocks containing transactions with invalid sequence numbers given the age of their inputs per BIP 68 are still accepted.
This pull-req is not a soft-fork nor does it contain any code to start that process.
This pull request builds on top of #6177.
Background: BIP 68, Consensus-enforced transaction replacement signalled via sequence numbers