This test has been growing too large, making it difficult to maintain. Especially when multiple pull requests change it.
ipc_util.pyinterface_ipc_mining.py, keeping only an interface sanity check in interface_ipc.pyinterface_ipc_mining.pyReview hint:
0git show --color-moved=dimmed-zebra --color-moved-ws=ignore-space-change
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
Reviewers, this pull request conflicts with the following ones:
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.
113+ ctx = capnp_modules['proxy'].Context()
114+ ctx.thread = thread
115+ return ctx, init
116+
117+
118+async def parse_and_deserialize_block(block_template, ctx):
In commit “test: move helpers to ipc_util.py” (766c008920c2d45516065e502a826b60d9e8f5f1)
Not important, but as long as these function signatures are being changed anyway (dropping self parameters) it would be nice to improve their current naming, which I think is misleading as have mentioned, and inconsistent with the names of the remote methods they call:
parse_and_deserialize_block -> get_blockparse_and_deserialize_coinbase_tx -> get_coinbase_raw_txparse_and_deserialize_coinbase -> get_coinbase_tx40+ yield obj
41+ finally:
42+ await obj.destroy(ctx)
43+
44+
45+async def create_block_template(mining, stack, ctx, opts):
In commit “test: move helpers to ipc_util.py” (766c008920c2d45516065e502a826b60d9e8f5f1)
May be good to move create_block_template and wait_next_template below near parse_and_deserialize_block so the mining functions are grouped together and the file is organized with generic async functions at the top, followed by capnproto functions, followed by mining functions.
289@@ -266,7 +290,15 @@ async def wait_for_block():
290 asyncio.run(capnp.run(async_routine()))
291
292 def run_test(self):
293+ self.miniwallet = MiniWallet(self.nodes[0])
In commit “test: split interface_ipc_mining.py” (9f80bd7c390409cc1713a0ccc58018bd801758a0)
Seems good to use a shared miniwallet variable like the shared opts variable, but also inconsistent to pass one as a parameter and attach other to self. It would seem better to pass them both as parameters so it’s clear which tests are using them.
ACK 836ed1fb88ed66bd966b35eb4aec69fb195a0f77
Nit: The commit messages could be a bit more descriptive.
106- blockRewardRemaining=int(template_capnp.blockRewardRemaining),
107- requiredOutputs=[bytes(output) for output in template_capnp.requiredOutputs],
108- lockTime=int(template_capnp.lockTime),
109- )
110+ src_dir = Path(self.config['environment']['SRCDIR']) / "src"
111+ self.capnp_modules = load_capnp_modules(src_dir, ["proxy", "init", "echo", "mining"])
In commit “test: move IPC helpers to ipc_util.py” (6ce4c3183e1be9d9f6af9c10d8a24734d16b61f8)
Not important, but it seems kludgy that load_capnp_modules caller is now responsible for listing the modules, but the list of modules also needs to be maintained inside the function. Also how the caller is responsible for figuring out the beginning of src_dir paths but the function is responsible for figuring out the end.
Would suggest reverting all changes to load_capnp_modules except giving it a config argument and calling load_capnp_modules(config) instead of self.load_capnp_modules()
Current implementation is also fine if preferred, though, just a suggestion
mempool_limit.py failure seems spurious.
Test
mempool_limit.pyfailure seems spurious.
There’s also a message “error: cannot rebase: Your index contains uncommitted changes.” that seem unexpected, but maybe it’s part of the “6 ancestors commits” expected output
129+
130+ async def async_routine():
131+ ctx, mining = await self.make_mining_ctx()
132+ blockref = await mining.getTip(ctx)
133+ current_block_height = self.nodes[0].getchaintips()[0]["height"]
134+ assert blockref.result.height == current_block_height
I understand it’s move-only, but some people prefer
0 assert_equal(blockref.result.height, current_block_height)
assert_equal for better error message?
You mentioned you prefer clean move-only and also suggested changes here https://github.com/bitcoin/bitcoin/commit/63434b1b26eb32f06b112a473a5e3259d1659daa#r2773665264.
I prefer separating refactors from commits introducing behavioral changes. I also prefer code moves be separated from everything else to simplify code review comparisons. There’s no contradiction - except that I personally don’t like assert_equal, but I know other do so I mentioned it here :)
163+
164+ self.log.debug("Test some inspectors of Template")
165+ header = (await template.getBlockHeader(ctx)).result
166+ assert_equal(len(header), block_header_size)
167+ block = await get_block(template, ctx)
168+ assert_equal(ser_uint256(block.hashPrevBlock), blockref.result.hash)
how come we’ve changes this from
0 assert_equal(ser_uint256(block.hashPrevBlock), newblockref.hash)
is it a result of a subtest split? Can we do these in separate commits to clarify?
89+ "echo": capnp.load(str(src_dir / "ipc" / "capnp" / "echo.capnp"), imports=imports),
90+ "mining": capnp.load(str(src_dir / "ipc" / "capnp" / "mining.capnp"), imports=imports),
91+ }
92+
93+
94+async def make_capnp_init_ctx(node, capnp_modules):
async def make_capnp_init_ctx(self): and changing it - can we do these in separate commit, it makes review a lot harder
get_block are close to RPCs so I would have preferred names that are easier to distinguish
nit: Adding to that some of these names like
get_blockare close to RPCs so I would have preferred names that are easier to distinguish
Makes sense now that they are global to add a mining_ prefix. Alternately, it could make sense to do import ipc_util and call ipc_util.get_block, ipc_util.get_coinbase_tx etc, but our current test style seems prefer global imports.
mining_ prefix.
insector
parse_and_deserialize_coinbase_tx was renamed here, but the code indicates it was parse_and_deserialize_coinbase (no _tx suffix)
ipc_mining.py
124+
125+ async def async_routine():
126+ ctx, init = await make_capnp_init_ctx(self.nodes[0], self.capnp_modules)
127+ self.log.debug("Create Mining proxy object")
128+ mining = init.makeMining(ctx).result
129+ blockref = await mining.getTip(ctx)
in the original we had a few more assertions:
0 self.log.debug("Test simple inspectors")
1 assert (await mining.isTestChain(ctx)).result
2 assert not (await mining.isInitialBlockDownload(ctx)).result
3 blockref = await mining.getTip(ctx)
4 assert blockref.hasResult
5 assert_equal(len(blockref.result.hash), block_hash_size)
Is this just moved elsewhere and we’re just avoiding duplication?
We’re mixing refactors here, makes review more difficult than it should be - could you split low-risk refactors from riskier or slightly more involved changes and explain the non-trivial changes in the commit message?
The simple inspector checks remain in interface_ipc.py, so they’re not moved.
I think the dimmed_zebra diff is more clear this way then if I were to temporariliy duplicate the code:
I’ll make this more clear in the commit message.
122+ return ctx, mining
123+
124 def run_mining_interface_test(self):
125- """Test Mining interface methods: waitTipChanged, createNewBlock, checkBlock."""
126- self.log.info("Running mining test")
127+ """Test Mining interface methods: getTip, waitTipChanged, checkBlock."""
checkBlock tested in run_coinbase_and_submission_test only?
I find the current refactors mixed with new changes harder to review that I think it needs to be.
Initially my idea was to change things if the line was touched anyway, e.g. going from self.some_method_with_ugly_name to some_method_with_pretty_name, but I agree it’s too noisy and some other lines are touched too. Let me try to split out another commit.
re: #34452#pullrequestreview-3762378213
Could we split out strict move-only changes (formatting and imports are fine) from adjusting method signatures and changing assertions and renaming, etc.
Seems good to make this PR easier to review, but I will say I’m surprised by these comments. I’d agree commit messages should be more accurate and tell reviewers what’s changing so they know exactly what to expect and to be surprised if they see any other changes. But the commits themselves seem fine.
In the main commit 3b47cc33fca2de2e2a4f684a0dec50c2c4a85435 every single line is shown as move-only by git except for import statements and 5 changed signatures:
0load_capnp_modules(self) -> load_capnp_modules(config)
1make_capnp_init_ctx(self) -> make_capnp_init_ctx(node, capnp_modules)
2parse_and_deserialize_block(self, block_template, ctx) -> get_block(block_template, ctx)
3get_coinbase_raw_tx(self, block_template, ctx) -> get_coinbase_raw_tx(block_template, ctx)
4parse_and_deserialize_coinbase(self, block_template, ctx) -> get_coinbase_tx(block_template, ctx)
So reviewing the commit feels completely trivial to me, it’s just a matter of making sure all the lines are move only, and the only lines that aren’t move-only are the simple substitutions above. I think I only spent 1-2 minutes reviewing it myself.
That said, it would be nice to split up, maybe by using @staticmethod to get rid of the self arguments and by defining global aliases load_capnp_modules = IPCInterfaceTest.load_capnp_modules so the signatures can change once in an initial commit and the followup commit can be 100% move-only.
Code review ACK 18b297c8aedc1923a3e6869406958afc372b2653
Maybe the changes could have been made a bit easier to review but I’ve seen much worse too. I couldn’t find any issues that I would consider a blocker.
So reviewing the commit feels completely trivial to me, it’s just a matter of making sure all the lines are move only
Since I expected others to review it like that, I did it differently, I have re-moved it in reverse and check what differences I got - which definitely wasn’t trivial, especially since the renames resulted in some duplicates that don’t translate back cleanly.
utACK 18b297c8aedc1923a3e6869406958afc372b2653
Maybe the changes could have been made a bit easier to review but I’ve seen much worse too. I couldn’t find any issues that I would consider a blocker.
I agree this is good as is, but I also agree with @l0rinc on untangling move-only commits from commits that make changes, even if the author doesn’t apply it this time. I think we should still point out specific improvements personally; those comments have improved me as a PR author. I’ve used them as a standard for subsequent PRs, so in a way, they ensure we maintain review standards and set the tone for what we accept.
Move IPC helpers into ipc_util.py and update interface_ipc.py
to use them.
Rename some helpers for clarity:
- parse_and_deserialize_block -> mining_get_block
- parse_and_deserialize_coinbase_tx -> mining_get_coinbase_tx
- get_coinbase_raw_tx -> mining_get_coinbase_raw_tx
- wait_next_template -> mining_wait_next_template
Split Mining interface tests into interface_ipc_mining.py and keep
interface_ipc.py for echo + simple inspectors.
Register the new test in test_runner.py.
The setup code around "Create Mining proxy object" is duplicated
in the new test file, but the simple insector checks below it
are not moved.
Split the Mining interface test into focused subtests.
Keep the initial tip-change pre-mine check in run_mining_interface_test.
As a result run_block_template_test no longer has newblockref.
- share miniwallet and block create options between tests
- documentation fixes
- use assert_equal instead of assert ==
@ryanofsky wrote:
That said, it would be nice to split up, maybe by using
@staticmethodto get rid of the self arguments and by defining global aliasesload_capnp_modules = IPCInterfaceTest.load_capnp_modulesso the signatures can change once in an initial commit and the followup commit can be 100% move-only
I missed this and leaving it for a followup, unless I have to make other more invasive changes.
25+ assert_not_equal
26+)
27+from test_framework.wallet import MiniWallet
28+from test_framework.ipc_util import (
29+ destroying,
30+ mining_create_block_template,
code review ACK 633d1831199a2ca6ca31b97312130419328d5479
There are still a few nits in commit messages and PR description and in the imports that I would prefer we fix, but based on my understanding of this area of the code, this split seems correct. Since my review, a few interface changes were reverted to simplify review, code splits were simplified, renamed shared IPC mining helpers to mining_*, assertion standardized, docstrings simplified or removed and commit messages were updated to contain additional info. Tests are skipped for me locally but the rest of the build passes after rebase.