joinpsbts previously took the minimum nLockTime across the PSBTs being merged, so the combined transaction could be minable earlier than the strictest party’s locktime. A merged PSBT should use the maximum nLockTime amongst the PSBTs that have at least one input with nSequence != SEQUENCE_FINAL.
PSBTs whose inputs are all SEQUENCE_FINAL do not constrain the locktime.
This PR implements the fix, updates RPC documentation, and extends rpc_psbt tests.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35100.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Stale ACK | ViniciusCestarii |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
1833 | + const bool locktime_enabled = std::any_of(psbtx.tx->vin.begin(), psbtx.tx->vin.end(), [](const CTxIn& txin) { 1834 | + return txin.nSequence != CTxIn::SEQUENCE_FINAL; 1835 | + }); 1836 | + if (locktime_enabled && (!has_locktime_constraint || psbtx.tx->nLockTime > max_locktime)) { 1837 | + max_locktime = psbtx.tx->nLockTime; 1838 | + has_locktime_constraint = true;
I don't understand why you need the has_locktime_constarint. If I am not wrong, remvoing it would not alter the logic of the if as it would still enter always that the current locktime is higher. I would suggest to eliminate it:
// Choose the highest lock time
if (locktime_enabled && psbtx.tx->nLockTime > max_locktime) {
max_locktime = psbtx.tx->nLockTime;
has_locktime_constraint is meant to be based on the logic found here:
It’s my understanding that not all inputs’ locktimes should contribute to the overall transaction locktime constraint. Specifically, if all inputs have nSequence set to SEQUENCE_FINAL, then nLockTime is ignored. So, in joinpsbts, I think we should only consider a PSBT’s nLockTime if it has at least one input with nSequence != SEQUENCE_FINAL.
Does that make sense, or am I still overlooking something?
Yes, if in al the inputs the nSequence is set to SEQUENCE_FINAL the locktime shouldn't be considered. However, I do not see how the variable has_locktime_constraint helps you checks this. This is done in locktime_enabled, which checks if at least one inputs doesn't have SEQUENCE_FINAL.
My logic tells me that has_locktime_constraint will just be needed the first time this condition is evaluated:
if (locktime_enabled && (!has_locktime_constraint || psbtx.tx->nLockTime > max_locktime)) {
The locktime_enabled is needed since it is checking that at least one input has SEQUENCE_FINAL.
The !has_locktime_constraint will always be true the first time since it is initialized to false and then it is changed to true after the first valid assignment.
On the other hand, the condition psbtx.tx->nLockTime > max_locktime will also typically be true on the first iteration, since max_locktime is initialized to 0. Therefore, almost any locktime will satisfy it. But even if the LockTime = 0, the behavior remains consistent, since max_locktime would remain unchanged.
What I am trying to say is that has_locktime_constraint is redundant since usually psbtx.tx->nLockTime > max_locktime will also usually be staisfied. And even when it is not statifaied (Locktime = 0), there is no need to enter in the if.
I hope I explained myself clearly
Okay, I see. I've updated the code.
923 | @@ -923,6 +924,29 @@ def test_psbt_input_keys(psbt_input, keys): 924 | assert "final_scriptwitness" not in joined_decoded['inputs'][3] 925 | assert "final_scriptSig" not in joined_decoded['inputs'][3] 926 | 927 | + # Check that joinpsbts ignores locktime when all inputs use SEQUENCE_FINAL 928 | + addr5 = self.nodes[1].getnewaddress("", "bech32m") 929 | + utxo5 = self.create_outpoints(self.nodes[0], outputs=[{addr5: 5}])[0] 930 | + self.generate(self.nodes[0], 6) 931 | + vin = [{"txid": utxo4["txid"], "vout": utxo4["vout"], "sequence": SEQUENCE_FINAL}] 932 | + psbt_50 = self.nodes[1].createpsbt(vin, {self.nodes[0].getnewaddress():Decimal('4.999')}, locktime=50)
Even if the sequence was valid, the psbt with locktime= 50 would be ignore as the maximum is 200. I think it is better to put a higher locktime than 200 to show the locktime is ignored
psbt_250 = self.nodes[1].createpsbt(vin, {self.nodes[0].getnewaddress():Decimal('4.999')}, locktime=250)
taken, thanks!
Left some comment son the code.
Still, I think it should be discuss how to approach the case where we have a height-based timelock and time-based timelock. If I am not wrong, it is considered height-based if 0 < locktime < 500_000_000, and if it is higher than that it is time-based.
With the current approach, if we had a locktime of each type, the time-based would always be the max_locktime, whether or not the height-based is older. I am not sure if this should be the expected behavior, or if it is at least better than what we currently have, which selects the min_locktime.
With the current approach, if we had a locktime of each type, the
time-basedwould always be themax_locktime, whether or not theheight-basedis older. I am not sure if this should be the expected behavior, or if it is at least better than what we currently have, which selects themin_locktime.
Thanks for pointing this out. Yea, with respect to this issue, I think it would make the most sense if we never tried to compare height-based locktimes and time-based locktimes (only compare "apples to apples"). I'll look more for examples of how the is handled elsewhere in the codebase.
Thanks for pointing this out. Yea, with respect to this issue, I think it would make the most sense if we never tried to compare height-based locktimes and time-based locktimes (only compare "apples to apples"). I'll look more for examples of how the is handled elsewhere in the codebase.
It may be enough for now since there is almost no usage of time-based locktimes (you can check the mainnet explorer to see the exact %). And it would be even less the % of psbt convined with diferent locktime. However, I think that ideally we should think a way to handel this cases, somthing like make the user choose one of the locktimes or set a new one, or maybe even set it to 0. I am not really sure which one would be the best option.
Yeah, the old min-locktime behavior was a real footgun. Anyone who signed first with a strict locktime would silently get overridden by whoever signed last with a looser one, which kind of defeats the whole point. Glad to see this fixed.
A couple of things on top of what @Bicaru20 already raised:
Agree that has_locktime_constraint is doing nothing once max_locktime starts at 0 and the locktime-enabled gate already filters out the all-SEQUENCE_FINAL PSBTs. The plain > comparison covers the first iteration just as well.
The height-vs-time locktime thing is worth discussing more before settling. A single tx only has one nLockTime field with one interpretation, so if some incoming PSBTs are height-based and others are time-based there's no merged value that respects everyone's intent. Picking the numeric max can produce a tx that semantically locks none of the original parties the way they expected. Not sure what the cleanest answer is, could be rejecting the join, could be something else, but it feels worth nailing down before this lands rather than leaving the behavior implicit.
One test case I'd add: all PSBTs have every input as SEQUENCE_FINAL, so nobody contributes a constraint and the merged tx should come out with nLockTime = 0. The current tests cover the "one is final-only, one isn't" case but not the "all final-only" one, and that's the path where max_locktime stays at its initial value, so it'd be nice to lock that down.
Tiny doc nit: the new help line is accurate but a reader who doesn't already know the rule might not get why SEQUENCE_FINAL matters. Tagging on something like "(PSBTs with all inputs final do not constrain the locktime)" would make the help readable on its own.
@GerardoTaboada Thanks for the feedback!
I've updated the code to address your concerns/suggestions. This implementation disallows joining pbsts with different locktime types.
tACK 077457b4103038cff8e781e7b09573ad73cbaa70 built and ran new functional tests and tried a single PSBT with mixed inputs (one SEQUENCE_FINAL, one non-final) and a time-based nLockTime, joined with a height-based PSBT.
This implementation disallows joining pbsts with different locktime types.
I'm still not conviced this is the right choice, specially if the current behavior is to allow psbts with diferent locktimes to be joined. For this case, I think it would be better to leave the psbt to 0 or leave the lowest locktime (as we do rigth now) and maybe throw a warning message so the user can see two different lockitmes are present in the psbts.
It would be nice to hear other people opinion on this.
There's precedent for this RPC to throw an error when something is ambiguous:
joinpsbts does this on duplicate inputs rawtransaction.cpp: two PSBTs claiming the same UTXO is a real conflict the RPC could paper over by dropping one, but it throws instead. Mixed height/time locktimes is the same shape of conflict in the same RPC, so rejecting is consistent.
It's about not producing a PSBT that can violate someone's explicit constraint.
Rejection forces the caller to confront the conflict explicitly and resolve it.
That said, the breaking change concern is valid. My intuition is that few callers actually rely on the current behavior of silently picking the lower locktime but I can't back that up with data, and I genuinely don't know what the project's bar is for a breaking change of this kind.
A merged PSBT should use the maximum `nLockTime` amongst the PSBTs that
have at least one input with `nSequence` != `SEQUENCE_FINAL`. PSBTs
whose inputs are all `SEQUENCE_FINAL` do not constrain the locktime.
Document this in the help text and extend rpc_psbt tests.
Updated to rebase off master and changed the structure of functional tests. I think this approach is easier to follow and best allows for modification if a different behavior is eventually chosen.