Skip to content

test: stabilize asset locks functional test #7411

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
PastaPastaPasta merged 1 commit into from
Jul 8, 2026
Merged

test: stabilize asset locks functional test #7411

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 37 additions & 44 deletions test/functional/feature_asset_locks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,15 +95,19 @@ def create_assetlock(self, coin, amount, pubkey):
return tx_from_hex(lock_tx["hex"])


def create_assetunlock_request_id(self, index):
# request ID = sha256("plwdtx", index)
request_id_buf = ser_string(b"plwdtx") + struct.pack("<Q", index)
return hash256(request_id_buf)[::-1].hex()


def create_assetunlock(self, index, withdrawal, pubkey=None, fee=tiny_amount):
node_wallet = self.nodes[0]
mninfo = self.mninfo
assert_greater_than(int(withdrawal), fee)
tx_output = CTxOut(int(withdrawal) - fee, key_to_p2pk_script(pubkey))

# request ID = sha256("plwdtx", index)
request_id_buf = ser_string(b"plwdtx") + struct.pack("<Q", index)
request_id = hash256(request_id_buf)[::-1].hex()
request_id = self.create_assetunlock_request_id(index)

height = node_wallet.getblockcount()
self.log.info(f"Creating asset unlock: index={index} {request_id}")
Expand Down Expand Up @@ -133,6 +137,24 @@ def create_assetunlock(self, index, withdrawal, pubkey=None, fee=tiny_amount):
unlock_tx.vExtraPayload = unlockTx_payload.serialize()
return unlock_tx


def create_assetunlock_for_oldest_quorum(self, start_index, withdrawal, pubkey):
expected_quorum_hash = self.nodes[0].quorum('list')['llmq_test_platform'][-1]
# Quorum selection depends on the request ID. Scan 100 candidates to
# avoid missing the target oldest quorum by chance.
for index in range(start_index, start_index + 100):

@chatgpt-codex-connector chatgpt-codex-connector Bot Jul 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Avoid scanning into indexes reused later

When this helper is called with start_index=501 or 601, the widened + 100 scan includes the hard-coded indexes reused later in the same test (520-522 and 620-622). If the first candidate that maps to the oldest quorum lands on one of those values, create_assetunlock() has already produced a recovered signature for that request ID; the later unlock with the same index but a different withdrawal has a different msgHash, so platformsign refuses it as a conflicting vote and the test times out/fails. Keep this search range disjoint from the later indexes or reserve the selected index.

Useful? React with 👍 / 👎.

@UdjinM6 UdjinM6 Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

same as #7409 (comment): is there a reason the range was bumped this much? nvm, I get it now

request_id = self.create_assetunlock_request_id(index)
quorum_hash = self.mninfo[0].get_node(self).quorum("selectquorum", llmq_type_test, request_id)["quorumHash"]
if quorum_hash == expected_quorum_hash:
self.log.info(f"Selected asset unlock index={index} for oldest active quorum {expected_quorum_hash}")
asset_unlock_tx = self.create_assetunlock(index, withdrawal, pubkey)
asset_unlock_tx_payload = CAssetUnlockTx()
asset_unlock_tx_payload.deserialize(BytesIO(asset_unlock_tx.vExtraPayload))
assert_equal(format(asset_unlock_tx_payload.quorumHash, '064x'), expected_quorum_hash)
return asset_unlock_tx, asset_unlock_tx_payload, expected_quorum_hash

raise AssertionError("Unable to select oldest active platform quorum")

def get_credit_pool_balance(self, node = None, block_hash = None):
if node is None:
node = self.nodes[0]
Expand Down Expand Up @@ -231,14 +253,14 @@ def send_tx(self, tx, expected_error = None, reason = None):
except JSONRPCException as e:
assert expected_error in e.error['message']

def generate_batch(self, count):
def generate_batch(self, count, sync_fun=None):
self.log.info(f"Generate {count} blocks")
while count > 0:
self.log.info(f"Generating batch of blocks {count} left")
batch = min(50, count)
count -= batch
self.bump_mocktime(10 * 60 + 1)
self.generate(self.nodes[1], batch)
self.generate(self.nodes[1], batch, sync_fun=sync_fun)

# This functional test intentionally setup only 2 MN and only 2 Evo nodes
# to ensure that corner case of quorum with minimum amount of nodes as possible
Expand Down Expand Up @@ -371,11 +393,12 @@ def test_asset_unlocks(self, node_wallet, node, pubkey):
self.check_mempool_result(tx=asset_unlock_tx_duplicate_index,
result_expected={'allowed': False, 'reject-reason' : 'bad-assetunlock-not-verified'})

self.log.info("Validating payload hash calculation by using hard-coded message hash")
self.log.info("Validating payload quorum selection")
asset_unlock_tx_payload = CAssetUnlockTx()
asset_unlock_tx_payload.deserialize(BytesIO(asset_unlock_tx.vExtraPayload))

assert_equal(asset_unlock_tx_payload.quorumHash, int(self.mninfo[0].get_node(self).quorum("selectquorum", llmq_type_test, 'e6c7a809d79f78ea85b72d5df7e9bd592aecf151e679d6e976b74f053a7f9056')["quorumHash"], 16))
request_id = self.create_assetunlock_request_id(101)
assert_equal(asset_unlock_tx_payload.quorumHash, int(self.mninfo[0].get_node(self).quorum("selectquorum", llmq_type_test, request_id)["quorumHash"], 16))

txid = self.send_tx(asset_unlock_tx)

Expand Down Expand Up @@ -451,7 +474,7 @@ def test_asset_unlocks(self, node_wallet, node, pubkey):
for inode in self.nodes:
inode.invalidateblock(block_asset_unlock)
self.validate_credit_pool_balance(locked)
self.generate_batch(25)
self.generate_batch(25, sync_fun=lambda: self.sync_blocks())

@knst knst Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

why this line is updated? how sync-mempools + sync-blocks is worse than just sync-blocks?

Is it for better performance? Or any other reason?

@thepastaclaw thepastaclaw Jul 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good question. This is intentional for the flake tracked in #7310: after invalidating block_asset_unlock, the disconnected asset-unlock txs can legitimately sit in different node mempools. This section only needs the replacement chain to advance and assert the credit-pool state stays at locked; mempool equality is not part of the invariant here.

The old default generate_batch() did sync_all(), and one of the recorded #7310 failures timed out exactly here in sync_mempools() with one node having txs and the others empty. Passing sync_blocks() keeps the chain-state synchronization needed by this reorg path without gating on nondeterministic mempool convergence.

@knst knst Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

one of the recorded #7310 failures timed out exactly here in sync_mempools()

Passing sync_blocks() keeps the chain-state synchronization needed by this reorg path without gating on nondeterministic mempool convergence.

Failed sync of mempool probably points out to some broken connections between masternodes or consensus diversification ; otherwise mempools will be synced. And removing sync_mempools on this stage would just cause it hang on the later steps at some point.

I'd suggest to revert this change.

Otherwise, PR looks fine for me.

@thepastaclaw thepastaclaw Jul 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I dug into this more, and I think the block-only sync here is intentional.

The important details are:

  1. Right before this invalidation path, the test already does a normal generate(...), which defaults to sync_all() (sync_blocks() + sync_mempools()). So the nodes should already be normally synced before invalidateblock().

  2. This invalidation is deep: block_asset_unlock is roughly 46 blocks below the tip (HEIGHT_DIFF_EXPIRING is 48, and the asset-unlock block is mined about two blocks after that height is recorded).

  3. Core only tries to resurrect disconnected transactions into the mempool for the first 10 disconnected blocks:

    (++disconnected <= 10)

    The nearby comment says that for deep invalidation, keeping the mempool up to date is probably futile.

  4. invalidateblock() is a local RPC path. Any tx resurrection is local AcceptToMemoryPool(...) cleanup; it is not a relay step that guarantees all peers converge to the same mempool.

So the thing this part of the test needs to prove is: after replacing the invalidated chain, all nodes accept the same blocks and keep the expected credit-pool state. sync_blocks() plus validate_credit_pool_balance(...) checks that. Requiring exact mempool equality immediately after a deep invalidation is asserting something Core does not guarantee.

The test does return to normal synchronization after reconsiderblock(): the later generate(...) uses default sync_all(), so mempool sync is not being skipped forever, only in the deep-invalidation window where it is not a reliable invariant.

I think the best follow-up is to keep this sync_blocks() call and add a short code comment explaining the deep-invalidation/mempool-resurrection reason.

self.validate_credit_pool_balance(locked)
for inode in self.nodes:
inode.reconsiderblock(block_to_reconsider)
Expand Down Expand Up @@ -658,24 +681,9 @@ def test_withdrawals_fork(self, node_wallet, node, pubkey):
assert softfork_active(node_wallet, 'withdrawals')
self.log.info(f'post-withdrawals height: {node.getblockcount()} credit: {self.get_credit_pool_balance()}')

index = 501
while index < 511:
self.log.info(f"Generating new Asset Unlock tx, index={index}...")
asset_unlock_tx = self.create_assetunlock(index, COIN, pubkey)
asset_unlock_tx_payload = CAssetUnlockTx()
asset_unlock_tx_payload.deserialize(BytesIO(asset_unlock_tx.vExtraPayload))

self.log.info("Check that Asset Unlock tx is valid for current quorum")
self.check_mempool_result(tx=asset_unlock_tx, result_expected={'allowed': True, 'fees': {'base': Decimal(str(tiny_amount / COIN))}})

quorumHash_str = format(asset_unlock_tx_payload.quorumHash, '064x')
assert quorumHash_str in node_wallet.quorum('list')['llmq_test_platform']

if quorumHash_str != node_wallet.quorum('list')['llmq_test_platform'][-1]:
self.log.info("The quorum for this msg-hash is not the last one in the list of active quorums. Try again!")
index += 1
else:
break
asset_unlock_tx, asset_unlock_tx_payload, quorumHash_str = self.create_assetunlock_for_oldest_quorum(501, COIN, pubkey)
self.log.info("Check that Asset Unlock tx is valid for current quorum")
self.check_mempool_result(tx=asset_unlock_tx, result_expected={'allowed': True, 'fees': {'base': Decimal(str(tiny_amount / COIN))}})

assert quorumHash_str in node_wallet.quorum('list')['llmq_test_platform']
self.log.info("Generate one more quorum to make signing quorum inactive but still valid")
Expand Down Expand Up @@ -710,24 +718,9 @@ def test_v24_fork(self, node_wallet, node, pubkey):
self.activate_by_name('v24', 750)
self.log.info(f'post-v24 height: {node.getblockcount()} credit: {self.get_credit_pool_balance()}')

index = 601
while index < 611:
self.log.info(f"Generating new Asset Unlock tx, index={index}...")
asset_unlock_tx = self.create_assetunlock(index, COIN, pubkey)
asset_unlock_tx_payload = CAssetUnlockTx()
asset_unlock_tx_payload.deserialize(BytesIO(asset_unlock_tx.vExtraPayload))

self.log.info("Check that Asset Unlock tx is valid for current quorum")
self.check_mempool_result(tx=asset_unlock_tx, result_expected={'allowed': True, 'fees': {'base': Decimal(str(tiny_amount / COIN))}})

quorumHash_str = format(asset_unlock_tx_payload.quorumHash, '064x')
assert quorumHash_str in node_wallet.quorum('list')['llmq_test_platform']

if quorumHash_str != node_wallet.quorum('list')['llmq_test_platform'][-1]:
self.log.info("The quorum for this msg-hash is not the last one in the list of active quorums. Try again!")
index += 1
else:
break
asset_unlock_tx, asset_unlock_tx_payload, quorumHash_str = self.create_assetunlock_for_oldest_quorum(601, COIN, pubkey)
self.log.info("Check that Asset Unlock tx is valid for current quorum")
self.check_mempool_result(tx=asset_unlock_tx, result_expected={'allowed': True, 'fees': {'base': Decimal(str(tiny_amount / COIN))}})

assert quorumHash_str in node_wallet.quorum('list')['llmq_test_platform']
self.log.info("Generate one more quorum to make signing quorum inactive but still valid")
Expand Down
34 changes: 19 additions & 15 deletions test/functional/test_framework/test_framework.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2095,24 +2095,25 @@ def check_dkg_session():

self.wait_until(check_dkg_session, timeout=timeout, sleep=sleep)

def node_has_quorum_commitment(self, node, quorum_hash, llmq_type):
s = node.quorum("dkgstatus")
if "minableCommitments" not in s:
return False
commits = s["minableCommitments"]
for c in commits:
if c["llmqType"] != llmq_type:
continue
if c["quorumHash"] != quorum_hash:
continue
if c["quorumPublicKey"] == '0' * 96:
continue
return True
return False

def wait_for_quorum_commitment(self, quorum_hash, mninfos, llmq_type=100, timeout=15):
def check_dkg_comitments():
for mn in mninfos:
s = mn.get_node(self).quorum("dkgstatus")
if "minableCommitments" not in s:
return False
commits = s["minableCommitments"]
c_ok = False
for c in commits:
if c["llmqType"] != llmq_type:
continue
if c["quorumHash"] != quorum_hash:
continue
if c["quorumPublicKey"] == '0' * 96:
continue
c_ok = True
break
if not c_ok:
if not self.node_has_quorum_commitment(mn.get_node(self), quorum_hash, llmq_type):
return False
return True

Expand Down Expand Up @@ -2201,6 +2202,9 @@ def mine_quorum(self, llmq_type_name="llmq_test", llmq_type=100, expected_connec
self.log.info("Waiting final commitment")
self.wait_for_quorum_commitment(q, mninfos_online, llmq_type=llmq_type)

self.log.info("Waiting final commitment on mining node")
self.wait_until(lambda: self.node_has_quorum_commitment(self.nodes[0], q, llmq_type), timeout=15)
Comment on lines 2203 to +2206

@coderabbitai coderabbitai Bot Jul 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Remove the retained all-masternode commitment wait.

Line 2203 still blocks on every mninfos_online node before Line 2206 checks the miner, so mine_quorum() remains gated on non-mining-node DKG propagation. For this stabilization path, wait only for the node that will build the commitment block.

Proposed fix 
-        self.log.info("Waiting final commitment")
-        self.wait_for_quorum_commitment(q, mninfos_online, llmq_type=llmq_type)
-
         self.log.info("Waiting final commitment on mining node")
         self.wait_until(lambda: self.node_has_quorum_commitment(self.nodes[0], q, llmq_type), timeout=15)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
self.wait_for_quorum_commitment(q, mninfos_online, llmq_type=llmq_type)
self.log.info("Waiting final commitment on mining node")
self.wait_until(lambda: self.node_has_quorum_commitment(self.nodes[0], q, llmq_type), timeout=15)
self.log.info("Waiting final commitment on mining node")
self.wait_until(lambda: self.node_has_quorum_commitment(self.nodes[0], q, llmq_type), timeout=15)
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/functional/test_framework/test_framework.py` around lines 2203 - 2206,
The quorum setup in mine_quorum still waits for every mninfos_online node to
receive the commitment before checking the miner, which keeps the mining path
gated on full DKG propagation. Remove the retained all-masternode wait and make
the flow in mine_quorum wait only for the mining node’s commitment, using
node_has_quorum_commitment on self.nodes[0] before proceeding.


self.log.info("Mining final commitment")
self.bump_mocktime(1)
self.nodes[0].getblocktemplate() # this calls CreateNewBlock
Expand Down
Loading