Repository: bitcoin/bitcoin
File: test/functional/interface_ipc.py (Python)

Code before:
# Copyright (c) The Bitcoin Core developers
# Distributed under the MIT software license, see the accompanying
# file COPYING or http://www.opensource.org/licenses/mit-license.php.
"""Test the IPC (multiprocess) interface."""
import asyncio
import inspect
from contextlib import asynccontextmanager, AsyncExitStack
from io import BytesIO
from pathlib import Path
import shutil
from test_framework.messages import (CBlock, CTransaction, ser_uint256, COIN)
from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import (
    assert_equal,
    assert_not_equal
)
from test_framework.wallet import MiniWallet

# Test may be skipped and not have capnp installed
try:
    import capnp  # type: ignore[import] # noqa: F401
except ImportError:
    pass

@asynccontextmanager
async def destroying(obj, ctx):
    """Call obj.destroy(ctx) at end of with: block. Similar to contextlib.closing."""
    try:
        yield obj
    finally:
        await obj.destroy(ctx)

async def create_block_template(mining, stack, ctx, opts):
    """Call mining.createNewBlock() and return template, then call template.destroy() when stack exits."""
    return await stack.enter_async_context(destroying((await mining.createNewBlock(opts)).result, ctx))

async def wait_next_template(template, stack, ctx, opts):
    """Call template.waitNext() and return template, then call template.destroy() when stack exits."""
    return await stack.enter_async_context(destroying((await template.waitNext(ctx, opts)).result, ctx))

async def wait_and_do(wait_fn, do_fn):
    """Call wait_fn, then sleep, then call do_fn in a parallel task. Wait for
    both tasks to complete."""
    wait_started = asyncio.Event()
    result = None

    async def wait():
        nonlocal result
        wait_started.set()
        result = await wait_fn


Review comment (bug):
re: https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3518142243

> It turns out that using the `getCoinbase()` result directly in the Python test led to memory management related errors. Parts of the coinbase were dropped by the time of the second `checkBlock()`.
>
> This probably happened once I extracted the coinbase specific checks into `build_coinbase_test`, which returns the coinbase `CTransaction` but presumably let go of the `getCoinbase()` result.
>
> Rather than debug (our use of) PyCapnp, I just introduced a `@dataclass` for `CoinbaseTemplateData`, so we fully own (everything in) it.

I looked into the [diff](https://github.com/bitcoin/bitcoin/compare/5f3883b7969826df85d97b2b4c475cae6a545f17..80acabaf9353245f0839b18d9552ab39f595eda6) and [CI failure](https://github.com/bitcoin/bitcoin/actions/runs/19269384275/job/55093373232?pr=33819#step:7:2962) described here and found this change pretty confusing because I couldn't see how the dataclass would make any difference. Introducing a dataclass would seem to just move around the invalid references without changing them.

The actual fix seems to be in the `parse_and_deserialize_coinbase` function which adds `bytes` casts. The `bytes` casts do nothing on my system which uses pycapnp-2.0.0, but in the CI job which uses pycapnp-2.2.1, it looks like casts have the effect of converting `memoryview` objects into `bytes` objects (https://github.com/capnproto/pycapnp/pull/380), fixing the crash that happens in CI.

I feel like the way the test works isn't very clear from looking at the code, and that the dataclass adds extra complexity. So a change like following could make sense:

<details><summary>diff</summary>
<p>

```diff
--- a/test/functional/interface_ipc.py
+++ b/test/functional/interface_ipc.py
@@ -4,7 +4,6 @@
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Test the IPC (multiprocess) interface."""
 import asyncio
-from dataclasses import dataclass
 from io import BytesIO
 from pathlib import Path
 import shutil
@@ -23,16 +22,6 @@ from test_framework.test_framework import (BitcoinTestFramework, assert_equal)
 from test_framework.wallet import MiniWallet
 
 
-@dataclass
-class CoinbaseTemplateData:
-    version: int
-    inputSequence: int
-    scriptSigPrefix: bytes
-    witness: Optional[bytes]
-    valueRemaining: int
-    outputs: list[bytes]
-    lockTime: int
-
 # Test may be skipped and not have capnp installed
 try:
     import capnp  # type: ignore[import] # noqa: F401
@@ -106,21 +95,6 @@ class IPCInterfaceTest(BitcoinTestFramework):
         tx.deserialize(coinbase_data)
         return tx
 
-    async def parse_and_deserialize_coinbase(self, block_template, ctx) -> CoinbaseTemplateData:
-        template_capnp = (await block_template.result.getCoinbase(ctx)).result
-        witness: Optional[bytes] = None
-        if template_capnp._has("witness"):
-            witness = bytes(template_capnp.witness)
-        return CoinbaseTemplateData(
-            version=int(template_capnp.version),
-            inputSequence=int(template_capnp.inputSequence),
-            scriptSigPrefix=bytes(template_capnp.scriptSigPrefix),
-            witness=witness,
-            valueRemaining=int(template_capnp.valueRemaining),
-            outputs=[bytes(output) for output in template_capnp.outputs],
-            lockTime=int(template_capnp.lockTime),
-        )
-
     def run_echo_test(self):
         self.log.info("Running echo test")
         async def async_routine():
@@ -137,21 +111,25 @@ class IPCInterfaceTest(BitcoinTestFramework):
 
     async def build_coinbase_test(self, template, ctx, miniwallet):
         self.log.debug("Build coinbase transaction using getCoinbase()")
-        coinbase_template = await self.parse_and_deserialize_coinbase(template, ctx)
+        # Note: the coinbase_template struct will be garbage-collected when this
+        # method returns, so it is important to copy any Data fields from it
+        # which need to be accessed later using the bytes() cast. Starting with
+        # pycapnp v2.2.0, Data fields have type `memoryview` and are ephemeral.
+        coinbase_template = (await template.result.getCoinbase(ctx)).result
         coinbase = CTransaction()
         coinbase.version = coinbase_template.version
         coinbase.vin = [CTxIn()]
         coinbase.vin[0].prevout = NULL_OUTPOINT
         coinbase.vin[0].nSequence = coinbase_template.inputSequence
         # Typically a mining pool appends its name and an extraNonce
-        coinbase.vin[0].scriptSig = coinbase_template.scriptSigPrefix
+        coinbase.vin[0].scriptSig = bytes(coinbase_template.scriptSigPrefix) # copy data with bytes()
 
         # We currently always provide a coinbase witness, even for empty
         # blocks, but this may change, so always check:
-        has_witness = coinbase_template.witness is not None
+        has_witness = coinbase_template._has("witness")
         if has_witness:
             coinbase.wit.vtxinwit = [CTxInWitness()]
-            coinbase.wit.vtxinwit[0].scriptWitness.stack = [coinbase_template.witness]
+            coinbase.wit.vtxinwit[0].scriptWitness.stack = [bytes(coinbase_template.witness)] # copy data with bytes()
 
         # First output is our payout
         coinbase.vout = [CTxOut()]
```
</p>
</details>

Alternately, if the current approach is preferred, it would at least be good to add some comments explaining the bytes() calls.

Code after:
# Copyright (c) The Bitcoin Core developers
# Distributed under the MIT software license, see the accompanying
# file COPYING or http://www.opensource.org/licenses/mit-license.php.
"""Test the IPC (multiprocess) interface."""
import asyncio
import inspect
from contextlib import asynccontextmanager, AsyncExitStack
from dataclasses import dataclass
from io import BytesIO
from pathlib import Path
import shutil
from test_framework.blocktools import NULL_OUTPOINT
from test_framework.messages import (
    CBlock,
    CTransaction,
    CTxIn,
    CTxOut,
    CTxInWitness,
    ser_uint256,
    COIN,
)
from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import (
    assert_equal,
    assert_not_equal
)
from test_framework.wallet import MiniWallet
from typing import Optional

# Stores the result of getCoinbaseTx()
@dataclass
class CoinbaseTxData:
    version: int
    sequence: int
    scriptSigPrefix: bytes
    witness: Optional[bytes]
    blockRewardRemaining: int
    requiredOutputs: list[bytes]
    lockTime: int

# Test may be skipped and not have capnp installed
try:
    import capnp  # type: ignore[import] # noqa: F401
except ImportError:
    pass

@asynccontextmanager
async def destroying(obj, ctx):
    """Call obj.destroy(ctx) at end of with: block. Similar to contextlib.closing."""
    try:
        yield obj