rpc: Merge joinpsbts locktimes correctly #35100

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:

https://github.com/bitcoin/bitcoin/blob/744d47fcee0d32a71154292699bfdecf954a6065/src/consensus/tx_verify.cpp#L24-L35

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.

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!

Test for non-final nLockTime = 0

<details> <summary>diff</summary>

diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py
index f441290210..55144be83c 100755
--- a/test/functional/rpc_psbt.py
+++ b/test/functional/rpc_psbt.py
@@ -1042,6 +1042,7 @@ class PSBTTest(BitcoinTestFramework):
 
         psbt_150 = make_locktime_psbt(150, locktime=150)
         psbt_200 = make_locktime_psbt(200, locktime=200)
+        psbt_0 = make_locktime_psbt(0, locktime=0)
         psbt_250_seq_final = make_locktime_psbt(250, locktime=250, sequence=SEQUENCE_FINAL)
         psbt_300_seq_final = make_locktime_psbt(300, locktime=300, sequence=SEQUENCE_FINAL)
         psbt_time = make_locktime_psbt(LOCKTIME_THRESHOLD, locktime=LOCKTIME_THRESHOLD)
@@ -1059,6 +1060,9 @@ class PSBTTest(BitcoinTestFramework):
         # joinpsbts uses the maximum time-based locktime
         assert_joined_locktime([psbt_time, psbt_later_time], expected_locktime=LOCKTIME_THRESHOLD + 1)
 
+        # joinpsbts ignores zero locktime even when nSequence is non-final
+        assert_joined_locktime([psbt_0, psbt_time], expected_locktime=LOCKTIME_THRESHOLD)
+
         # joinpsbts ignores SEQUENCE_FINAL locktimes before rejecting mixed locktime types
         assert_joined_locktime([psbt_250_seq_final, psbt_time], expected_locktime=LOCKTIME_THRESHOLD)

</details>

The CLTV regression test from #35326 can be added here too.

<details> <summary>diff</summary>

diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py
index f441290210..b5eddfd727 100755
--- a/test/functional/rpc_psbt.py
+++ b/test/functional/rpc_psbt.py
@@ -5,9 +5,11 @@
 """Test the Partially Signed Transaction RPCs.
 """
 from decimal import Decimal
+from io import BytesIO
 from itertools import product
 from random import randbytes
 
+from test_framework.address import script_to_p2wsh
 from test_framework.blocktools import (
     MAX_STANDARD_TX_WEIGHT,
 )
@@ -17,6 +19,7 @@ from test_framework.messages import (
     COutPoint,
     CTransaction,
     CTxIn,
+    CTxInWitness,
     CTxOut,
     MAX_BIP125_RBF_SEQUENCE,
     SEQUENCE_FINAL,
@@ -41,7 +44,15 @@ from test_framework.psbt import (
     PSBT_OUT_TAP_TREE,
     PSBT_OUT_SCRIPT,
 )
-from test_framework.script import CScript, LOCKTIME_THRESHOLD, OP_TRUE, SIGHASH_ALL, SIGHASH_ANYONECANPAY
+from test_framework.script import (
+    CScript,
+    LOCKTIME_THRESHOLD,
+    OP_CHECKLOCKTIMEVERIFY,
+    OP_DROP,
+    OP_TRUE,
+    SIGHASH_ALL,
+    SIGHASH_ANYONECANPAY,
+)
 from test_framework.script_util import MIN_STANDARD_TX_NONWITNESS_SIZE
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
@@ -470,6 +481,56 @@ class PSBTTest(BitcoinTestFramework):
         assert_raises_rpc_error(-8, "The PSBT version can only be 2 or 0", self.nodes[0].converttopsbt, hexstring=rawtx, psbt_version=1)
         assert_raises_rpc_error(-8, "The PSBT version can only be 2 or 0", self.nodes[0].psbtbumpfee, txid=tobump, psbt_version=1)
 
+    def test_joinpsbts_cltv_locktime(self):
+        self.log.info("Test that joining PSBTs preserves the higher CLTV height locktime")
+        node = self.nodes[0]
+        amount = Decimal("1.0")
+        spend_amount = Decimal("0.999")
+        sequence = 0xfffffffe
+        current_height = node.getblockcount()
+        locktimes = [current_height - 2, current_height - 1]
+
+        witness_scripts = [
+            CScript([locktime, OP_CHECKLOCKTIMEVERIFY, OP_DROP])
+            for locktime in locktimes
+        ]
+        utxos = self.create_outpoints(node, outputs=[
+            {script_to_p2wsh(witness_script): amount}
+            for witness_script in witness_scripts
+        ])
+        self.generate(node, 1)
+
+        psbts = [
+            node.createpsbt(
+                inputs=[{**utxo, "sequence": sequence}],
+                outputs=[{node.getnewaddress(): spend_amount}],
+                locktime=locktime,
+                replaceable=False,
+                psbt_version=0,
+            )
+            for utxo, locktime in zip(utxos, locktimes)
+        ]
+
+        joined = node.joinpsbts(psbts)
+        joined_psbt = PSBT.from_base64(joined)
+        joined_tx = CTransaction()
+        joined_tx.deserialize(BytesIO(joined_psbt.g.map[PSBT_GLOBAL_UNSIGNED_TX]))
+        assert_equal(joined_tx.nLockTime, max(locktimes))
+
+        # joinpsbts may reorder inputs, so attach each CLTV witness script by prevout.
+        scripts_by_prevout = {
+            (int(utxo["txid"], 16), utxo["vout"]): witness_script
+            for utxo, witness_script in zip(utxos, witness_scripts)
+        }
+        joined_tx.wit.vtxinwit = [CTxInWitness() for _ in joined_tx.vin]
+        for i, txin in enumerate(joined_tx.vin):
+            joined_tx.wit.vtxinwit[i].scriptWitness.stack = [
+                CScript([OP_TRUE]),
+                scripts_by_prevout[(txin.prevout.hash, txin.prevout.n)],
+            ]
+        test_accept = node.testmempoolaccept([joined_tx.serialize().hex()], maxfeerate=0)[0]
+        assert_equal(test_accept["allowed"], True)
+
     def run_test(self):
         # Create and fund a raw tx for sending 10 BTC
         psbtx1 = self.nodes[0].walletcreatefundedpsbt([], {self.nodes[2].getnewaddress():10})['psbt']
@@ -1065,6 +1126,7 @@ class PSBTTest(BitcoinTestFramework):
         # joinpsbts rejects mixed height-based and time-based locktimes
         assert_raises_rpc_error(-8, "Cannot join PSBTs with mixed height-based and time-based locktimes", self.nodes[0].joinpsbts, [psbt_150, psbt_time])
         assert_raises_rpc_error(-8, "Cannot join PSBTs with mixed height-based and time-based locktimes", self.nodes[0].joinpsbts, [psbt_time, psbt_150])
+        self.test_joinpsbts_cltv_locktime()
 
         # Check that joining shuffles the inputs and outputs
         # 10 attempts should be enough to get a shuffled join

</details>

This is a good test case, please add it in a separate commit.

This assignment seems redundant, the following line can use the constant directly.

Since it's a bool, s/locktime_enabled/is_locktime_enabled.

The default for optional is being assigned too early and being used in the condition below. For presence type checks, the has_value() notation reads better imho.

Same for this variable as well - the sentinel value is used where an optional can be helpful for readability.

This is long file and the run_test function spans over 1000 lines including this diff!

Let's avoid adding more code directly in this function and split out the functionality in a function named test_joinpsbts like few functions at the end of run_test do. If inclined, the existing 4 call sites of joinpsbts can also go in there.