Skip to content

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Jul 12, 2022

Not sure why the python constant is named differently than the constant in the C++ source code.

Especially, if we use this in context of MAX+1 (#25575 (comment)) the rename makes sense, in my eyes.

@fanquake fanquake requested a review from glozow July 12, 2022 16:10
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

Note that the test rpc_psbt.py actually also defines MAX_BIP125_RBF_SEQUENCE, i.e. I'd propose the following additional patch after the scripted-diff:

diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py
index d2a888fd3..264b2ac32 100755
--- a/test/functional/rpc_psbt.py
+++ b/test/functional/rpc_psbt.py
@@ -11,8 +11,9 @@ from itertools import product
 from test_framework.descriptors import descsum_create
 from test_framework.key import ECKey, H_POINT
 from test_framework.messages import (
-    ser_compact_size,
+    MAX_BIP125_RBF_SEQUENCE,
     WITNESS_SCALE_FACTOR,
+    ser_compact_size,
 )
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
@@ -27,7 +28,6 @@ from test_framework.wallet_util import bytes_to_wif
 import json
 import os

-MAX_BIP125_RBF_SEQUENCE = 0xfffffffd

 # Create one-input, one-output, no-fee transaction:
 class PSBTTest(BitcoinTestFramework):

MacroFake added 2 commits July 12, 2022 18:49
…_SEQUENCE

-BEGIN VERIFY SCRIPT-
 sed -i 's:BIP125_SEQUENCE_NUMBER:MAX_BIP125_RBF_SEQUENCE:g' $(git grep -l BIP125_SEQUENCE_NUMBER ./test)
-END VERIFY SCRIPT-
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-review ACK faace13

@DrahtBot
Copy link
Contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #20892 (tests: Run both descriptor and legacy tests within a single test invocation by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Copy link
Member

@glozow glozow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

concept ACK faace13

@fanquake fanquake requested a review from instagibbs July 13, 2022 15:01
@maflcko maflcko merged commit 31c6309 into bitcoin:master Jul 13, 2022
@maflcko maflcko deleted the 2207-test-scrpt-rbf-🐴 branch July 13, 2022 15:30
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 13, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Jul 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants