feat: improve generation script, add wallet export, add tests#1
Conversation
📝 WalkthroughWalkthroughRefactors generator into a strategy-driven class system (wallet-sync, random), adds wallet export utilities and a standalone export script, enhances dashd lifecycle and RPC client behavior, changes error types, adds a dashd setup helper, updates .gitignore for Python, and introduces unit and integration tests. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
actor User
end
participant CLI as CLI
participant Gen as Generator\n(strategy)
participant Dashd as DashdManager
participant RPC as DashRPCClient
participant FS as Filesystem
User->>CLI: invoke generate.py / export_wallets.py
CLI->>Gen: instantiate chosen strategy
Gen->>Dashd: start(dashd_executable, extra_args, rpc_port)
Dashd->>Dashd: verify executable, set preexec limits
Dashd->>RPC: create RPC client (rpc_port, datadir)
RPC->>Dashd: poll getblockcount until ready
Dashd-->>Gen: return rpc client & temp dir
Gen->>Gen: run strategy phases / generate txs
Gen->>RPC: create/broadcast txs, mine blocks
RPC->>FS: write chainstate / wallet files
Gen->>Gen: _collect_wallet_statistics()
Gen->>FS: _save_wallet_files() -> write per-wallet JSON
Gen->>Dashd: shutdown
Dashd-->>CLI: cleanup complete
CLI-->>User: finished
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
generator/rpc_client.py (1)
36-50:⚠️ Potential issue | 🟡 Minor
call()implicitly returnsNoneifmax_retries≤ 0.If
max_retriesis set to 0,range(0)produces an empty loop and the method silently returnsNoneinstead of making any RPC call. This is unlikely with the default of 3, but a defensive guard would prevent subtle bugs from misconfiguration.Proposed fix
def call(self, method: str, *params, wallet: str | None = None) -> Any: """ Call RPC method with retry logic and comprehensive error handling. """ + if self.max_retries < 1: + return self._execute(method, params, wallet) for attempt in range(self.max_retries):
🤖 Fix all issues with AI agents
In `@export_wallets.py`:
- Around line 91-98: preexec_fn currently tries to setrlimit to a hardcoded
(10000,10000) which may exceed the system hard limit and silently fails; update
preexec_fn to read the current hard limit via
resource.getrlimit(resource.RLIMIT_NOFILE), cap the desired value at that hard
limit (like dashd_manager.py does), and then call resource.setrlimit with the
min-capped values; also change the subprocess.Popen call for proc to not discard
dashd errors—use stderr=subprocess.PIPE (or otherwise preserve stderr) instead
of subprocess.DEVNULL so startup errors are visible; reference
functions/symbols: preexec_fn, resource.getrlimit/resource.setrlimit, and
subprocess.Popen/stderr.
- Around line 100-104: The cleanup function should accept an optional exit_code
(default 0), call proc.terminate() and proc.wait(timeout=10) inside a try/except
catching subprocess.TimeoutExpired and in that except call proc.kill() then
proc.wait() to ensure the process is stopped; finally call sys.exit(exit_code).
Update all failure call sites that currently call cleanup() to pass a non-zero
code (e.g., cleanup(1)) while success paths keep cleanup() or cleanup(0). Refer
to the cleanup function and callers that invoke cleanup on success/failure and
to the proc variable used inside cleanup.
- Around line 4-5: The import of the Unix-only module resource in
export_wallets.py will crash on Windows; change the top-level import to a
guarded import (try/except ImportError) so resource is optional (set to None on
import failure) and then update the preexec_fn that references resource to check
that resource is not None before calling any resource.* functions (or skip the
resource-specific calls when None); ensure the unique symbol preexec_fn in this
file is adjusted to conditionally apply resource.setrlimit only when the
resource module was successfully imported.
In `@generate.py`:
- Around line 940-961: The stats["blocks_generated"] field is inconsistent
across strategies; make it represent only blocks generated by the generator loop
(not absolute chain height). In _initialize_utxo_pool, remove or replace the
assignment self.stats["blocks_generated"] = self.rpc.call("getblockcount") with
a value that counts only generation-loop blocks (e.g., 0 or the number of
bootstrap blocks explicitly tracked as "bootstrap_blocks"), and likewise update
WalletSyncGenerator where it sets stats["blocks_generated"] to the chain height
(lines referencing WalletSyncGenerator) so those places either stop assigning
absolute heights or store bootstrap height under a different key; ensure
RandomGenerator._generate_blocks continues to increment
stats["blocks_generated"] so the summary at the end reports the same semantic
meaning across strategies.
In `@generator/dashd_manager.py`:
- Around line 126-136: preexec_fn currently attempts
resource.setrlimit(RLIMIT_NOFILE, (target, hard)) and silently swallows OSError
when soft>hard; change the logic to compute new_soft = min(target, hard) and
new_hard = min(hard, target) (or handle RLIM_INFINITY appropriately) so you
never call setrlimit with soft>hard, then call
resource.setrlimit(resource.RLIMIT_NOFILE, (new_soft, new_hard)) inside the
try/except in preexec_fn (still catching OSError) to ensure the soft limit is
raised up to the available hard limit when the hard limit is below the target.
🧹 Nitpick comments (8)
generator/errors.py (1)
17-19: Type annotation inconsistency:code: int = Noneshould useint | None = None.The rest of the PR consistently uses PEP 604 union types (e.g.,
str | Noneinrpc_client.py,dashd_manager.py). This parameter should match for consistency.Proposed fix
- def __init__(self, message: str, code: int = None): + def __init__(self, message: str, code: int | None = None):generator/wallet_export.py (1)
82-83: Specify explicitencoding="utf-8"for cross-platform consistency.Without an explicit encoding,
open()uses the platform default (which may not be UTF-8 on Windows).Proposed fix
- with open(output_path, "w") as f: + with open(output_path, "w", encoding="utf-8") as f:tests/test_generate.py (2)
259-278: Test validates the concept but not the actual implementation.This test re-implements the weighted selection logic inline (
random.choices(tier_wallets, weights=weights, k=1)) rather than exercising the actual_create_transactionmethod or any generator method that performs wallet selection. If the generator's weighting logic diverges from this test, the test would still pass.Consider testing the actual generator method (e.g., calling
_create_transactionwith a mock RPC) or at minimum adding a comment noting that this tests the algorithm in isolation, not the integration.
420-423: Redundantimport pytest— already imported at Line 7.Proposed fix
if __name__ == "__main__": - import pytest - pytest.main([__file__, "-v"])generate.py (4)
196-206: Consider usingabc.ABCand@abstractmethodinstead of manualNotImplementedError.The base
Generatorusesraise NotImplementedErrorto enforce the subclass contract. Python'sabcmodule would enforce this at instantiation time rather than at call time, catching missing implementations earlier. The test at Line 374-386 intest_generate.pydirectly instantiatesGenerator(config), which would need adjustment if ABC were used, but it would provide a stronger contract.This isn't urgent since the current pattern works and is tested.
245-295: Shutdown sequence in_export_datadirectly manipulatesdashd_managerinternals.Lines 271-278 manually terminate the process and set
self.dashd_manager.process = None, bypassing theDashdManager.stop()method. This duplicates logic and could diverge asstop()evolves (e.g., it already has a kill fallback that this code lacks). Consider extracting astop_process_only()method onDashdManageror callingstop()with a flag to skip temp dir cleanup.
1198-1263: Localcurrent_heightcan drift from actual chain height.The bulk generation loop tracks
current_heightlocally and increments it manually. Extra blocks from boundary transactions (Line 1260-1261), periodic sends (Line 1271-1272), and faucet self-sends (Line 1283-1284) are accounted for, but any RPC failure between_mine_blocksand the increment could cause drift. The final check at Lines 1299-1305 catches undershoots but may overshoot without correction.Consider periodically syncing with the actual height (e.g., every N iterations):
Proposed addition inside the while loop
+ # Periodically sync with actual chain height to prevent drift + if current_height % 5000 == 0: + actual = self.rpc.call("getblockcount") + if actual != current_height: + current_height = actual + # Progress logging
117-130:KeyboardInterruptis caught and re-raised asGeneratorError, then also caught at Line 1449.Line 119 wraps
KeyboardInterruptintoGeneratorError("User interrupted"). This will be caught byexcept GeneratorErrorat Line 1446, printing"ERROR: User interrupted"and exiting with code 4. The separateexcept KeyboardInterruptat Line 1449 only fires if the interrupt happens outsidegenerate(). This works but the double handling is slightly confusing — the user sees"ERROR: User interrupted"rather than the expected"\nInterrupted by user".
xdustinface
force-pushed
the
feat/strategy
branch
from
84962d2 to
555653f
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@export_wallets.py`:
- Around line 76-101: The subprocess.Popen call that starts dashd (using
variables cmd and dashd_preexec_fn) can raise FileNotFoundError if the dashd
executable is missing; wrap the Popen invocation in a try/except that catches
FileNotFoundError (and optionally OSError), log or print a user-friendly error
message indicating the dashd executable was not found and include the attempted
executable name (dashd_executable), then exit with a non-zero code just like
DashdManager.start() does; ensure cleanup() is not registered/executed if
startup fails and keep the existing proc-related teardown logic for the
successful startup path.
- Around line 46-57: The check for network_subdir treats mainnet (where
network_subdir_name == "") the same as other networks causing a confusing
message like "No / subdirectory found"; update the logic around datadir /
network_subdir to special-case mainnet: if args.network == "mainnet" skip the
redundant existence check (since datadir was already validated) or change the
error message to omit the trailing slash and network name when
network_subdir_name is empty; modify the code that builds/validates
network_subdir (variables network_subdirs, network_subdir_name, network_subdir)
to either bypass the second exists() check for mainnet or produce a clear
message mentioning "mainnet" instead of an empty subdirectory.
In `@generate.py`:
- Around line 1219-1248: The code currently increments
self.stats["transactions_created"] when calling self._mine_blocks for coinbase
rewards in the mature/immature coinbase branches (using variables
mature_coinbase_start, immature_coinbase_start and coinbase_wallet_addr), which
incorrectly counts mined coinbase rewards as user transactions; change this by
incrementing a new counter like self.stats["coinbase_rewards"] (or similarly
named) instead of self.stats["transactions_created"] in both places where
wallet_blocks is added, and ensure the rest of the code/reporting uses
transactions_created only for actual sendtoaddress/sendmany calls.
In `@generator/wallet_export.py`:
- Around line 36-45: The list comprehension building utxos can raise KeyError
for missing keys; update the construction that iterates over wallet_utxos to use
safe .get() calls: replace u["address"] with u.get("address") (or
u.get("address", None) if you prefer explicit default) and replace
u["confirmations"] with u.get("confirmations", 0) so utxos creation in the utxos
variable is defensive against UTXOs missing those fields.
🧹 Nitpick comments (11)
generator/rpc_client.py (2)
36-50: Inconsistent exception chaining:from Nonevsfrom e.Line 45 uses
from None(suppresses original traceback) while Line 49 usesfrom e(preserves it). For timeouts, the originalTimeoutExpiredexception contains useful diagnostic info (command, timeout duration). Consider usingfrom econsistently for both paths.Proposed fix
- raise RPCError(f"RPC timeout after {self.rpc_timeout}s: {method}", code=-1) from None + raise RPCError(f"RPC timeout after {self.rpc_timeout}s: {method}", code=-1) from e
52-73:if self.rpc_port:is falsy for port 0.
if self.rpc_port:will skip appending-rpcport=0if port is0. While port 0 is unlikely for RPC, the idiomatic check should beif self.rpc_port is not None:to match theint | Nonetype annotation. Same applies toif self.datadir:on Line 56 (empty string vs None), andif wallet:on Line 62.Proposed fix
- if self.datadir: + if self.datadir is not None: cmd.append(f"-datadir={self.datadir}") - if self.rpc_port: + if self.rpc_port is not None: cmd.append(f"-rpcport={self.rpc_port}") - if wallet: + if wallet is not None: cmd.append(f"-rpcwallet={wallet}")generator/wallet_export.py (1)
82-83: Specifyencoding="utf-8"for portable JSON output.
open(output_path, "w")uses the platform default encoding, which may not be UTF-8 on all systems (e.g., Windows). Since JSON should be UTF-8, explicitly specify the encoding.Proposed fix
- with open(output_path, "w") as f: + with open(output_path, "w", encoding="utf-8") as f: json.dump(export_data, f, indent=2)generator/dashd_manager.py (1)
155-159:atexit.register(self.stop)can accumulate handlers.If
start()were called more than once (e.g., after astop()), each call registers another atexit handler, leading to redundant cleanup attempts. Not likely in current usage but worth guarding against.Proposed fix
+ # Unregister any previous cleanup handler before registering a new one + atexit.unregister(self.stop) atexit.register(self.stop)export_wallets.py (1)
20-30:find_free_portduplicates logic fromDashdManagerwithoutSO_REUSEADDR.
DashdManager.is_port_available(dashd_manager.py Line 58) setsSO_REUSEADDRbefore binding, which avoids false negatives on recently-closed sockets. This local version doesn't. Consider either reusingDashdManager's method or adding the socket option.Proposed fix
def find_free_port(start=19998): import socket for port in range(start, start + 20): try: with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: + s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) s.bind(("127.0.0.1", port)) return port except OSError: continue raise RuntimeError("No free port found")generate.py (4)
43-63: Config default probabilities don't match any named density preset.The dataclass defaults (
tx_probability_none=0.50,tx_probability_low=0.70) don't correspond to any of the named density configs inmain()(minimal,light,normal,heavy). Whilemain()always overrides these, directConfig()construction (e.g., in tests) silently gets a density profile that doesn't match any labeled preset. Consider aligning defaults with "normal" (0.30/0.70/0.90) for consistency.Proposed fix
# Transaction generation parameters (used by RandomGenerator) - tx_probability_none: float = 0.50 # Probability of 0 transactions per block + tx_probability_none: float = 0.30 # Probability of 0 transactions per block tx_probability_low: float = 0.70 # Cumulative probability for 1-3 transactions
196-206: Consider usingabc.ABCand@abstractmethodfor the base Generator.The base
Generatorusesraise NotImplementedErrorto enforce subclassing. Usingabc.ABCand@abstractmethodwould provide a clearer contract and catch missing implementations at instantiation time rather than at call time.strategy_name()(Line 132) also returns"base"rather than being abstract.
267-293:_export_databypassesDashdManager.stop()to manually manage the process.Lines 271-278 directly call
self.dashd_manager.process.terminate()and setprocess = None, duplicating stop logic and bypassing theatexit-registered handler. IfDashdManager.stop()gains additional cleanup (e.g., flushing, logging), this code path would miss it. Consider adding astop_process_only()method toDashdManagerthat stops the process without cleaning up the temp dir, keeping lifecycle management in one place.
1152-1296:_phase_bulk_generationis complex but well-structured.This ~150-line method handles multiple interleaved concerns (boundary transactions, coinbase mining ranges, periodic sends, progress logging). While it's long, the inline comments are clear and the logic is sequential. If this grows further, consider extracting the mature/immature coinbase ranges into helper methods.
tests/test_generate.py (2)
259-278: Test duplicates the weighting logic rather than exercising the actual method.
test_heavy_receives_more_transactionsre-implements therandom.choicesweighting logic fromRandomGenerator._create_transaction(Lines 787-794 in generate.py) instead of calling the method itself. This validates the concept but won't catch regressions if the weight values change in the production code.Since
_create_transactionrequires RPC calls, this is a reasonable pragmatic choice for a unit test. Consider adding a comment to that effect.
420-423: Redundantimport pytest.
pytestis already imported at Line 7.Proposed fix
if __name__ == "__main__": - import pytest - pytest.main([__file__, "-v"])
xdustinface
force-pushed
the
feat/strategy
branch
from
555653f to
dc4166b
Compare
xdustinface
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@contrib/setup-dashd.py`:
- Line 9: Update the module docstring to reflect the correct default DASHVERSION
(change "22.0.0" to "23.0.2") so it matches the actual default defined later;
edit the top-level docstring text referencing DASHVERSION in
contrib/setup-dashd.py to show the correct version string used by the default on
line where DASHVERSION is set.
- Around line 37-38: The Linux branch always returns the x86_64 asset; update
the logic that builds the filename to inspect platform.machine() (same approach
used for Darwin) and return "dashcore-{DASHVERSION}-aarch64-linux-gnu.tar.gz"
when machine() reports "aarch64" or "arm64", otherwise return the existing
x86_64 name; modify the conditional around system == "Linux" in
contrib/setup-dashd.py to handle ARM64 vs x86_64 using platform.machine() and
the DASHVERSION symbol.
In `@generator/rpc_client.py`:
- Around line 47-50: Remove the dead except ConnectionRefusedError branch in the
retry loop that calls subprocess.run (the block catching ConnectionRefusedError
and sleeping/raising DashdConnectionError), because subprocess.run invoking
dash-cli will not raise ConnectionRefusedError; instead errors are handled by
_handle_error which raises DashdConnectionError. Either delete that except
clause entirely from the function containing the subprocess.run call, or (if you
want retry-on-dashd-failure) change the retry logic to catch
DashdConnectionError raised by _handle_error and implement the backoff there;
locate references to subprocess.run, the ConnectionRefusedError except block,
_handle_error, and DashdConnectionError to make the change.
🧹 Nitpick comments (9)
generate.py (2)
677-786: Bulk generation loop manually trackscurrent_height— subtle drift risk.
current_heightis incremented manually after each_mine_blockscall rather than re-queried viagetblockcount. Whilegeneratetoaddressin regtest is deterministic, any unexpected behavior (e.g.,_send_to_wallet→_mine_blocks(1)at Line 739-740 adding an extra block) could cause the local counter to drift from the actual chain height.The overshoot detection at Lines 778-784 mitigates this for the final height, but mid-loop drift could cause boundary transactions to land at slightly wrong heights.
This is acceptable for test data generation but worth being aware of.
108-121:KeyboardInterruptre-raised asGeneratorError— caught again by the outer handler.In
generate(),KeyboardInterruptis caught and re-raised asGeneratorError("User interrupted"). Inmain()(Line 880), theGeneratorErrorhandler catches this and exits with code 4. However, Line 883-884 also has a separateKeyboardInterrupthandler. If the interrupt happens outsidegenerate()(e.g., during config setup), it'll be caught by the outer handler. This is fine, but thefrom Noneon Line 110 suppresses the original traceback which may hinder debugging.tests/test_generate.py (1)
166-169: Duplicateimport pytestat module-level entry point.
pytestis already imported at Line 6. The re-import on Line 167 is harmless but redundant.Proposed fix
if __name__ == "__main__": - import pytest - pytest.main([__file__, "-v"])tests/test_integration.py (1)
99-107: Module-scoped fixture is efficient but consider a longer timeout for CI.The
generated_datafixture runs--blocks 200with the default 120s timeout. On slower CI runners this could be tight. Consider bumping the timeout inrun_generatefor this call.Proposed fix
- result = run_generate("--blocks", "200", "--output-dir", str(output_dir)) + result = run_generate("--blocks", "200", "--output-dir", str(output_dir), timeout=300)export_wallets.py (2)
20-30:find_free_portduplicatesDashdManager.find_free_portwithoutSO_REUSEADDR.This standalone version omits the
SO_REUSEADDRsocket option thatDashdManager.is_port_available(dashd_manager.py Line 58) sets. This could cause false negatives on recently-freed ports. Consider extracting the port-finding logic into a shared utility or reusingDashdManager.
76-89: Empty-string sentinel for mainnet network flag is fragile.Building the command with an empty string and then filtering it out works, but is easy to break if someone adds arguments between. A cleaner approach:
Proposed fix
cmd = [ dashd_executable, - f"-{args.network}" if args.network != "mainnet" else "", f"-datadir={datadir}", f"-port={p2p_port}", f"-rpcport={rpc_port}", "-server=1", "-daemon=0", "-rpcbind=127.0.0.1", "-rpcallowip=127.0.0.1", "-listen=0", ] - # Remove empty strings from cmd (mainnet needs no network flag) - cmd = [c for c in cmd if c] + if args.network != "mainnet": + cmd.insert(1, f"-{args.network}")contrib/setup-dashd.py (1)
57-63: Ruff S202 flags onlytarfile.extractall(), notzipfile.extractall(); both still need path validation for defense-in-depth.The code extracts archives from GitHub releases (trusted source, low risk), but to mitigate path-traversal vulnerabilities: use
tarfile.extractall(dest_dir, filter="data")on Python 3.12+ for tar archives, and validatezipfile.ZipFile.namelist()members before extraction for zip archives.generator/rpc_client.py (2)
36-50:call()silently returnsNonewhenmax_retries <= 0.If
max_retriesis set to 0 (or negative),range(self.max_retries)produces an empty sequence, the loop body never executes, andcall()returnsNoneimplicitly. A guard at the top would make the contract explicit.Suggested guard
def call(self, method: str, *params, wallet: str | None = None) -> Any: """ Call RPC method with retry logic and comprehensive error handling. """ + if self.max_retries <= 0: + raise ValueError("max_retries must be >= 1") for attempt in range(self.max_retries):
59-60:if self.rpc_port:skips port0— useis not Nonefor correctness.Falsy check means
rpc_port=0would be silently ignored. While port 0 is unlikely for RPC, the intent is clearly "was a port explicitly provided," whichis not Noneexpresses accurately.- if self.rpc_port: + if self.rpc_port is not None:
- Refactor `generate.py` into a base `Generator` class with one strategy: `WalletSyncGenerator` (SPV wallet sync edge cases). - Extract shared wallet stats collection into `generator/wallet_export.py`. - Add `export_wallets.py` for exporting stats from existing blockchain data. - Improve dashd_manager extra_args support and rpc_client error handling. - Add test suite with unit and integration tests to make sure the script is working.
xdustinface
force-pushed
the
feat/strategy
branch
from
dc4166b to
267abb8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@contrib/setup-dashd.py`:
- Around line 58-64: The extract function currently calls
tarfile.open(...).extractall(...) which is vulnerable to path traversal;
implement a safe extraction path check and use the
tarfile.extractall(filter=...) parameter when supported. Inside extract, create
a predicate (e.g., is_safe_member(member, dest_dir)) that normalizes and joins
member.name with dest_dir and rejects absolute paths or any path that escapes
dest_dir (contains '..' after normpath or startswith outside dest_dir); then
call tf.extractall(dest_dir, filter=is_safe_member) inside a try block and fall
back in except TypeError by iterating tf.getmembers(), applying the same
is_safe_member check, and extracting only those members with tf.extract(member,
dest_dir). Reference the extract function, tarfile.open, tf.extractall, and
tf.extract/getmembers in your changes.
🧹 Nitpick comments (5)
export_wallets.py (1)
114-124: Bareexcept Exceptionis intentional here but worth a brief comment.The polling loop catches all exceptions during startup because the RPC client can raise various error types while dashd is initializing. A brief inline comment would clarify intent for future readers.
generate.py (1)
677-786: Manualcurrent_heighttracking may drift from actual chain height.Throughout
_phase_bulk_generation,current_heightis maintained manually by adding block counts after each_mine_blockscall. If any code path (e.g., the faucet self-send at Line 758-765 or the periodic sends at Line 744-755) fails to properly account for mined blocks, the local counter will drift from the actual chain height. The final reconciliation at Lines 777-784 mitigates this for the end result, but intermediate boundary/coinbase logic could misfire.This isn't a blocking issue given the reconciliation, but consider periodically syncing
current_heightwithself.rpc.call("getblockcount")(e.g., every N iterations) for robustness.tests/test_integration.py (1)
21-27: Preferpytest.skip()overassertfor missing environment prerequisites.When
DASHD_PATHis not set,assertraisesAssertionError, which pytest reports as a test failure rather than a skip. Usingpytest.skip()gives a clearer signal in CI and local runs that the test was intentionally skipped due to a missing prerequisite.Suggested change
def get_dashd_path(): path = os.environ.get("DASHD_PATH", "") - assert path, "DASHD_PATH environment variable is not set (run contrib/setup-dashd.py first)" + if not path: + pytest.skip("DASHD_PATH environment variable is not set (run contrib/setup-dashd.py first)") dashd = Path(path) - assert dashd.is_file(), f"dashd binary not found at {path}" - assert os.access(path, os.X_OK), f"dashd binary not executable at {path}" + if not dashd.is_file(): + pytest.skip(f"dashd binary not found at {path}") + if not os.access(path, os.X_OK): + pytest.skip(f"dashd binary not executable at {path}") return pathtests/test_generate.py (2)
148-163: Class name is misleading — these tests check initial state and method existence, not address indices.Consider renaming to something like
TestWalletSyncInitializationto better reflect what's being tested.
166-169: Duplicateimport pytest— already imported at line 6.Proposed fix
if __name__ == "__main__": - import pytest - pytest.main([__file__, "-v"])
1 participant
generate.pyinto a baseGeneratorclass with one strategy:WalletSyncGenerator(SPVwallet sync edge cases).
generator/wallet_export.py.export_wallets.pyfor exporting stats from existing blockchain data.Summary by CodeRabbit
New Features
Tests
Chores