feat: support mining transactions with shielded outputs - #167
Conversation
📝 WalkthroughWalkthroughDockerfile and pyproject.toml now use local Changes
Sequence Diagram(s)(Skipped — changes are primarily dependency/Docker updates, documentation, and tests; no new runtime control flow introduced that requires a sequence diagram.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 2
🧹 Nitpick comments (2)
tests/test_shielded_outputs.py (2)
56-60: Consider deterministic fake proof data for reproducibility.Using
os.urandom()generates different byte patterns on each test run. While this likely won't cause issues since proofs aren't validated in these tests, deterministic values improve reproducibility and debuggability.♻️ Optional: Use deterministic fake proofs
-# Fake range proof (~675 bytes in reality, use short placeholder for tests) -FAKE_RANGE_PROOF = os.urandom(675) - -# Fake surjection proof -FAKE_SURJECTION_PROOF = os.urandom(130) +# Fake range proof (~675 bytes in reality, use deterministic placeholder for tests) +FAKE_RANGE_PROOF = b'\xde' * 675 + +# Fake surjection proof +FAKE_SURJECTION_PROOF = b'\xad' * 130Then you can also remove the
import oson line 17.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_shielded_outputs.py` around lines 56 - 60, The tests use non-deterministic fake proof data via os.urandom for FAKE_RANGE_PROOF and FAKE_SURJECTION_PROOF which hurts reproducibility; replace those assignments to deterministic byte sequences (e.g., constant repeated bytes of the correct lengths) so each test run uses the same data, and remove the now-unused import os; update the symbols FAKE_RANGE_PROOF and FAKE_SURJECTION_PROOF in tests/test_shielded_outputs.py accordingly.
310-318: Consider extracting the polling loop into a helper.The same polling pattern is repeated in 6 tests. Extracting it would reduce duplication and make timeout behavior consistent.
♻️ Optional: Extract polling helper
async def _poll_until_done(self, job_id: str, max_attempts: int = 50, interval: float = 0.1) -> dict: """Poll job status until done or max attempts reached.""" for _ in range(max_attempts): await asyncio.sleep(interval) resp = await self.client.request("GET", "/job-status", params={"job-id": job_id}) data = await resp.json() if data["status"] == "done": return data return data # Return last status even if not doneThen in tests:
data = await self._poll_until_done(job_id) self.assertEqual("done", data["status"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_shielded_outputs.py` around lines 310 - 318, Extract the repeated polling loop into a helper method (e.g., async def _poll_until_done(self, job_id: str, max_attempts: int = 50, interval: float = 0.1) -> dict) that encapsulates the asyncio.sleep + self.client.request("GET", "/job-status", params={"job-id": job_id}) + await resp.json() loop and returns the final JSON (return early when data["status"] == "done" or the last status after max attempts); then replace the duplicated loop instances in the six tests in tests/test_shielded_outputs.py with a call to self._poll_until_done(job_id) and assert the returned data["status"] == "done", keeping configurable max_attempts/interval arguments as needed for timeout tuning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile`:
- Around line 14-16: The Dockerfile's copied locations don't match the relative
path in tx-mining-service's pyproject.toml (path = "../hathor-core/hathorlib");
update the Dockerfile so the source path expected by pyproject is present at
build time—for example change the COPY of hathor-core/hathorlib to the root
location expected by pyproject (replace the current COPY hathor-core/hathorlib/
/code/hathor-core/hathorlib/ with a COPY that places the library at
/hathor-core/hathorlib/) or alternatively place the tx-mining-service
pyproject.toml inside /code/tx-mining-service/ so its ../hathor-core/hathorlib
resolves to /code/hathor-core/hathorlib; modify the COPY lines referencing
pyproject.toml/poetry.lock and hathorlib accordingly to keep the filesystem
layout consistent with the pyproject.toml path.
In `@pyproject.toml`:
- Line 28: Document that pyproject.toml declares a local path dependency on
../hathor-core/hathorlib and require the repo layout with a sibling hathor-core
directory in the README (show the directory tree and note that Dockerfile COPY
expects ../hathor-core); update the CI workflows (docker.yml and main.yml) to
checkout the sibling repository before build/test by adding an actions/checkout
step for the parent/sibling repo (or checking out the parent directory) so
poetry install and Dockerfile COPY succeed during CI.
---
Nitpick comments:
In `@tests/test_shielded_outputs.py`:
- Around line 56-60: The tests use non-deterministic fake proof data via
os.urandom for FAKE_RANGE_PROOF and FAKE_SURJECTION_PROOF which hurts
reproducibility; replace those assignments to deterministic byte sequences
(e.g., constant repeated bytes of the correct lengths) so each test run uses the
same data, and remove the now-unused import os; update the symbols
FAKE_RANGE_PROOF and FAKE_SURJECTION_PROOF in tests/test_shielded_outputs.py
accordingly.
- Around line 310-318: Extract the repeated polling loop into a helper method
(e.g., async def _poll_until_done(self, job_id: str, max_attempts: int = 50,
interval: float = 0.1) -> dict) that encapsulates the asyncio.sleep +
self.client.request("GET", "/job-status", params={"job-id": job_id}) + await
resp.json() loop and returns the final JSON (return early when data["status"] ==
"done" or the last status after max attempts); then replace the duplicated loop
instances in the six tests in tests/test_shielded_outputs.py with a call to
self._poll_until_done(job_id) and assert the returned data["status"] == "done",
keeping configurable max_attempts/interval arguments as needed for timeout
tuning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5254e07b-d112-4b88-95ea-8f1966055324
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Dockerfilepyproject.tomltests/test_shielded_outputs.py
| # Cap required: setuptools >=82 removed pkg_resources, which is used by pycoin (a transitive dep via hathorlib) | ||
| setuptools = ">=68.0,<82" | ||
| hathorlib = {version = "^0.14.1", extras = ["client"]} | ||
| hathorlib = {path = "../hathor-core/hathorlib", develop = true, extras = ["client"]} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if README mentions the hathor-core dependency requirement
rg -i "hathor-core|companion|sibling" README.md || echo "No mention of hathor-core dependency found in README"Repository: HathorNetwork/tx-mining-service
Length of output: 127
🏁 Script executed:
# Check for Dockerfile and CI configuration
find . -type f \( -name "Dockerfile*" -o -name "*.yml" -o -name "*.yaml" \) | grep -E "(Dockerfile|\.github|ci)" | head -20Repository: HathorNetwork/tx-mining-service
Length of output: 147
🏁 Script executed:
# Look for CI configuration files
git ls-files | grep -E "\.github/workflows|\.circleci|\.travis|jenkinsfile" | head -10Repository: HathorNetwork/tx-mining-service
Length of output: 142
🏁 Script executed:
# Search for hathor-core references in Docker/CI files
rg "hathor-core" -g "Dockerfile*" -g "*.yml" -g "*.yaml" 2>/dev/null || echo "No hathor-core references in config files"Repository: HathorNetwork/tx-mining-service
Length of output: 537
🏁 Script executed:
cat -n Dockerfile | head -50Repository: HathorNetwork/tx-mining-service
Length of output: 1297
🏁 Script executed:
cat -n .github/workflows/docker.ymlRepository: HathorNetwork/tx-mining-service
Length of output: 2593
🏁 Script executed:
cat -n .github/workflows/main.yml | head -80Repository: HathorNetwork/tx-mining-service
Length of output: 1334
🏁 Script executed:
git ls-files | grep -E "\.gitmodules|\.git/config"Repository: HathorNetwork/tx-mining-service
Length of output: 57
🏁 Script executed:
cat -n .gitmodules 2>/dev/null || echo "No .gitmodules file found"Repository: HathorNetwork/tx-mining-service
Length of output: 100
🏁 Script executed:
# Check git config for submodules
git config --file .git/config --name-only --get-regexp path 2>/dev/null || echo "No submodules in git config"Repository: HathorNetwork/tx-mining-service
Length of output: 89
🏁 Script executed:
# Check if docker.yml has checkout steps we haven't seen
rg -A 30 "uses: docker/build-push-action" .github/workflows/docker.ymlRepository: HathorNetwork/tx-mining-service
Length of output: 324
🏁 Script executed:
# Check poetry.lock to see how hathorlib is handled
head -100 poetry.lock | grep -A 5 -B 5 "hathorlib" || echo "Checking further in poetry.lock..."Repository: HathorNetwork/tx-mining-service
Length of output: 109
🏁 Script executed:
# Search for hathorlib anywhere in poetry.lock
rg "hathorlib" poetry.lock || echo "No hathorlib entry found"Repository: HathorNetwork/tx-mining-service
Length of output: 126
🏁 Script executed:
cat -n .github/workflows/docker.yml | grep -A 5 -B 5 "actions/checkout"Repository: HathorNetwork/tx-mining-service
Length of output: 57
🏁 Script executed:
# Check the full workflow structure to see if hathor-core is cloned
rg -B 20 "docker/build-push-action" .github/workflows/docker.yml | head -40Repository: HathorNetwork/tx-mining-service
Length of output: 1029
Local path dependency and CI configuration needs attention.
The Dockerfile correctly documents the local path dependency on hathor-core (lines 11-13), but this requirement is not documented in the README. More critically, the CI workflows require the hathor-core repository to be available:
- The
docker.ymlworkflow doesn't explicitly checkout the parent directory or hathor-core, which is needed for the COPY command in the Dockerfile to succeed during builds. - The
main.ymltest workflow only checks out the current repo, sopoetry installwill fail when it tries to resolve the local path dependency../hathor-core/hathorlib.
Add documentation in the README for the required repository structure (directory layout with hathor-core as a sibling directory) and update CI workflows to clone both repositories appropriately before attempting to build or test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pyproject.toml` at line 28, Document that pyproject.toml declares a local
path dependency on ../hathor-core/hathorlib and require the repo layout with a
sibling hathor-core directory in the README (show the directory tree and note
that Dockerfile COPY expects ../hathor-core); update the CI workflows
(docker.yml and main.yml) to checkout the sibling repository before build/test
by adding an actions/checkout step for the parent/sibling repo (or checking out
the parent directory) so poetry install and Dockerfile COPY succeed during CI.
| [[package]] | ||
| name = "hathorlib" | ||
| version = "0.14.1" | ||
| version = "0.14.0" |
There was a problem hiding this comment.
question: We're downgrading the hathorlib?
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| class TestDevMinerShieldedOutputs(AioHTTPTestCase): |
There was a problem hiding this comment.
⭐ Thanks for testing the DevMiner right from the first iteration! It would be really easy to skip this newly added feature.
Use local hathorlib (from ../hathor-core/hathorlib) which includes ShieldedOutputsHeader, AmountShieldedOutput, and FullShieldedOutput types. Update Dockerfile to copy hathorlib source into the build context (build from parent directory). Add 21 tests covering serialization round-trips, tx parsing, PoW solving, dev-miner HTTP API lifecycle, and TxJob handling for shielded transactions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
52c3c81 to
a54f59e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/test_shielded_outputs.py (1)
311-320: Extract the/job-statuspolling and fail fast on terminal errors.This same wait loop is duplicated six times, and it currently burns the full timeout even after the job has already moved to a terminal failure state. A helper that returns on
"done"and callsself.fail(...)for"failed","cancelled", or"timeout"will make CI failures much easier to diagnose.♻️ Proposed helper
+ async def _wait_until_done(self, job_id: str) -> dict: + for _ in range(50): + await asyncio.sleep(0.1) + resp = await self.client.request( + "GET", "/job-status", params={"job-id": job_id} + ) + self.assertEqual(200, resp.status) + data = await resp.json() + status = data["status"] + if status in {"failed", "cancelled", "timeout"}: + self.fail(data["message"] or f"job {job_id} ended with {status}") + if status == "done": + return data + self.fail(f"job {job_id} did not finish in time")Also applies to: 339-349, 364-373, 387-396, 425-435, 452-462
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_shielded_outputs.py` around lines 311 - 320, Extract the duplicated polling loop into a helper (e.g., _wait_for_job_done) that accepts self and job_id, performs repeated calls to self.client.request("GET", "/job-status", params={"job-id": job_id}), returns immediately when data["status"] == "done", and calls self.fail(...) as soon as data["status"] is any terminal error ("failed", "cancelled", "timeout"); replace each duplicated loop (the blocks referencing job_id and asserting "done") with a call to this helper to fail fast and centralize polling logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile`:
- Around line 11-20: The README.md and docker-compose.dev-miner.yml must be
updated to reflect the new build context requirement used by the Dockerfile (see
COPY hathor-core/hathorlib/ and WORKDIR /code/tx-mining-service in the
Dockerfile); change the documented build command to "docker build -f
tx-mining-service/Dockerfile -t tx-mining-service ." (run from the repository
parent) or explicitly state "run docker build from the parent directory so
../hathor-core/hathorlib is in context", and add a note to verify/create a
.dockerignore in the parent directory to avoid sending unnecessary files to the
daemon during the build.
- Line 30: The runtime Dockerfile RUN that installs libgcc currently uses "RUN
apk add libgcc" which leaves the package index in the final image; change the
instruction in the Dockerfile (the RUN apk add libgcc line) to include
--no-cache (e.g., RUN apk add --no-cache libgcc) so the package cache isn't
stored in the final image.
In `@tests/test_shielded_outputs.py`:
- Around line 97-98: The test uses separate current-time calls when setting
tx.timestamp (via txstratum.time.time()) causing flaky mismatches; fix
TestTxJobShielded.test_txjob_uuid_includes_shielded_data() by computing a single
shared timestamp variable (e.g., now = int(txstratum.time.time())) and assigning
tx.timestamp = now for both the plain and shielded constructions (and reuse that
same now when computing expected hashes/UUIDs); apply the same change to the
other affected test blocks referenced (around the other ranges) so both variants
use the identical pinned timestamp.
---
Nitpick comments:
In `@tests/test_shielded_outputs.py`:
- Around line 311-320: Extract the duplicated polling loop into a helper (e.g.,
_wait_for_job_done) that accepts self and job_id, performs repeated calls to
self.client.request("GET", "/job-status", params={"job-id": job_id}), returns
immediately when data["status"] == "done", and calls self.fail(...) as soon as
data["status"] is any terminal error ("failed", "cancelled", "timeout"); replace
each duplicated loop (the blocks referencing job_id and asserting "done") with a
call to this helper to fail fast and centralize polling logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3fcfd461-7d21-43d9-b04e-92ff1fd3f4c8
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Dockerfilepyproject.tomltests/test_shielded_outputs.py
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
| # Copy hathorlib source (local path dependency: ../hathor-core/hathorlib). | ||
| # Build from the parent directory: | ||
| # docker build -f tx-mining-service/Dockerfile -t tx-mining-service . | ||
| COPY hathor-core/hathorlib/ /code/hathor-core/hathorlib/ | ||
|
|
||
| # Install in a subdirectory so the relative path "../hathor-core/hathorlib" | ||
| # in pyproject.toml resolves correctly to /code/hathor-core/hathorlib/. | ||
| COPY tx-mining-service/poetry.lock tx-mining-service/pyproject.toml /code/tx-mining-service/ | ||
|
|
||
| WORKDIR /code/tx-mining-service |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 \
--glob 'README*' \
--glob 'docker-compose*' \
--glob 'compose.*' \
--glob 'Dockerfile*' \
--glob 'Makefile*' \
--glob '*.sh' \
--glob '*.yml' \
--glob '*.yaml' \
'docker build|buildx build|context:|dockerfile:|tx-mining-service/Dockerfile'Repository: HathorNetwork/tx-mining-service
Length of output: 1005
Update documentation: build command now requires parent directory context.
The Dockerfile correctly documents the new build requirement (line 13: docker build -f tx-mining-service/Dockerfile -t tx-mining-service . from parent), but README.md (line 23) and docker-compose.dev-miner.yml (line 24) still document the old build command without the -f flag. Users following these instructions will encounter COPY hathor-core/hathorlib/ failed errors because the build context lacks the required parent directories.
Update README.md to show: docker build -f tx-mining-service/Dockerfile -t tx-mining-service . (run from parent) or document the parent context requirement explicitly. Verify .dockerignore exists in the parent directory to avoid sending unnecessary files to the daemon on each build.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dockerfile` around lines 11 - 20, The README.md and
docker-compose.dev-miner.yml must be updated to reflect the new build context
requirement used by the Dockerfile (see COPY hathor-core/hathorlib/ and WORKDIR
/code/tx-mining-service in the Dockerfile); change the documented build command
to "docker build -f tx-mining-service/Dockerfile -t tx-mining-service ." (run
from the repository parent) or explicitly state "run docker build from the
parent directory so ../hathor-core/hathorlib is in context", and add a note to
verify/create a .dockerignore in the parent directory to avoid sending
unnecessary files to the daemon during the build.
| COPY --from=build /usr/local/lib/python3.11/site-packages /usr/local/lib/python3.11/site-packages | ||
| # hathorlib is installed in develop mode (.pth file points to this path) | ||
| COPY --from=build /code/hathor-core/hathorlib /code/hathor-core/hathorlib | ||
| RUN apk add libgcc |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "Dockerfile" -type fRepository: HathorNetwork/tx-mining-service
Length of output: 87
🏁 Script executed:
cat -n ./DockerfileRepository: HathorNetwork/tx-mining-service
Length of output: 1685
Add --no-cache to the runtime apk add command.
The build stage (line 6) already uses --no-cache with apk add. The runtime stage should follow the same pattern to avoid storing the package index in the final image.
Proposed fix
-RUN apk add libgcc
+RUN apk add --no-cache libgcc📝 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.
| RUN apk add libgcc | |
| RUN apk add --no-cache libgcc |
🧰 Tools
🪛 Trivy (0.69.3)
[error] 30-30: 'apk add' is missing '--no-cache'
'--no-cache' is missed: apk add libgcc
Rule: DS-0025
(IaC/Dockerfile)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dockerfile` at line 30, The runtime Dockerfile RUN that installs libgcc
currently uses "RUN apk add libgcc" which leaves the package index in the final
image; change the instruction in the Dockerfile (the RUN apk add libgcc line) to
include --no-cache (e.g., RUN apk add --no-cache libgcc) so the package cache
isn't stored in the final image.
| # Update timestamp to current time | ||
| tx.timestamp = int(txstratum.time.time()) |
There was a problem hiding this comment.
Pin the timestamp in the hash/UUID comparisons.
Both assertions build the plain and shielded variants with separate current-time calls. If that crosses a one-second boundary, the hashes differ because of the timestamp alone, so these tests can pass without proving the shielded header participates in hashing.
💡 Proposed fix
-def _build_tx_with_shielded_outputs(shielded_outputs: list) -> bytes:
+def _build_tx_with_shielded_outputs(
+ shielded_outputs: list, *, timestamp: int | None = None
+) -> bytes:
@@
- tx.timestamp = int(txstratum.time.time())
+ tx.timestamp = int(txstratum.time.time()) if timestamp is None else timestamp+ timestamp = 1_700_000_000
tx_plain = tx_or_block_from_bytes(BASE_TX_DATA)
- tx_plain.timestamp = int(txstratum.time.time())
+ tx_plain.timestamp = timestamp
tx_plain.update_hash()
@@
- tx_bytes = _build_tx_with_shielded_outputs(outputs)
+ tx_bytes = _build_tx_with_shielded_outputs(outputs, timestamp=timestamp)Apply the same shared timestamp in TestTxJobShielded.test_txjob_uuid_includes_shielded_data().
Also applies to: 247-265, 485-498
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_shielded_outputs.py` around lines 97 - 98, The test uses separate
current-time calls when setting tx.timestamp (via txstratum.time.time()) causing
flaky mismatches; fix TestTxJobShielded.test_txjob_uuid_includes_shielded_data()
by computing a single shared timestamp variable (e.g., now =
int(txstratum.time.time())) and assigning tx.timestamp = now for both the plain
and shielded constructions (and reuse that same now when computing expected
hashes/UUIDs); apply the same change to the other affected test blocks
referenced (around the other ranges) so both variants use the identical pinned
timestamp.
…-amount shielded tests Update pyproject.toml, poetry.lock, and Dockerfile so the local hathorlib path dependency resolves to the correct sibling checkout. Add 13 tests covering transparent output amounts across the 4-byte to 8-byte encoding boundary (100M, 2**31-1, 2**31, 100B, 100T, 2**62, MAX_OUTPUT_VALUE) in combination with shielded outputs, exercising round-trip serialization, PoW solving, and full dev-miner HTTP submission. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
Dockerfile (1)
30-30:⚠️ Potential issue | 🟡 MinorUse
--no-cachefor the runtimeapk add.Line 30 still leaves the APK package index in the final image.
Proposed fix
-RUN apk add libgcc +RUN apk add --no-cache libgcc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` at line 30, The Dockerfile RUN line installs libgcc but leaves the APK index in the final image; update the RUN that contains "apk add libgcc" to use the no-cache option so the package index is not persisted (i.e., replace the existing "apk add libgcc" invocation with an apk add command that includes --no-cache to prevent storing the APK index).tests/test_shielded_outputs.py (1)
89-116:⚠️ Potential issue | 🟡 MinorPin the timestamp in hash/UUID inclusion tests.
These assertions can still pass for the wrong reason: the plain and shielded transactions are built with separate
time()calls, so a one-second boundary can make the hash/UUID differ even if shielded headers are not included in the hashed data.Proposed fix
def _build_tx_with_shielded_outputs( shielded_outputs: list, transparent_output_value: int | None = None, + timestamp: int | None = None, ) -> bytes: @@ - # Update timestamp to current time - tx.timestamp = int(txstratum.time.time()) + # Update timestamp to current time unless the caller pins it for comparisons. + tx.timestamp = int(txstratum.time.time()) if timestamp is None else timestampdef test_shielded_outputs_affect_hash(self): @@ + timestamp = int(txstratum.time.time()) + # Transaction without shielded outputs tx_plain = tx_or_block_from_bytes(BASE_TX_DATA) - tx_plain.timestamp = int(txstratum.time.time()) + tx_plain.timestamp = timestamp tx_plain.update_hash() hash_plain = tx_plain.hash # Same transaction with shielded outputs outputs = [_make_amount_shielded_output()] - tx_bytes = _build_tx_with_shielded_outputs(outputs) + tx_bytes = _build_tx_with_shielded_outputs(outputs, timestamp=timestamp)def test_txjob_uuid_includes_shielded_data(self): """The job UUID (tx hash) differs between shielded and non-shielded versions.""" + timestamp = int(txstratum.time.time()) + # Non-shielded tx_plain = tx_or_block_from_bytes(BASE_TX_DATA) - tx_plain.timestamp = int(txstratum.time.time()) + tx_plain.timestamp = timestamp tx_plain.update_hash() job_plain = TxJob(bytes(tx_plain)) # Shielded outputs = [_make_amount_shielded_output()] - tx_bytes = _build_tx_with_shielded_outputs(outputs) + tx_bytes = _build_tx_with_shielded_outputs(outputs, timestamp=timestamp) job_shielded = TxJob(tx_bytes)Also applies to: 256-274, 494-507
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_shielded_outputs.py` around lines 89 - 116, The tests race because timestamps are taken separately; update _build_tx_with_shielded_outputs to accept an optional timestamp parameter (e.g., timestamp: int | None = None), set tx.timestamp = timestamp if provided (otherwise keep current int(time.time())), and modify callers in the hash/UUID inclusion tests to call both the plain and shielded builders with the same pinned timestamp value so hashes/UUIDs are deterministic; apply the same pattern to the other helper usages referenced (around the other occurrences noted) so all paired builds share the same timestamp.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Dockerfile`:
- Line 30: The Dockerfile RUN line installs libgcc but leaves the APK index in
the final image; update the RUN that contains "apk add libgcc" to use the
no-cache option so the package index is not persisted (i.e., replace the
existing "apk add libgcc" invocation with an apk add command that includes
--no-cache to prevent storing the APK index).
In `@tests/test_shielded_outputs.py`:
- Around line 89-116: The tests race because timestamps are taken separately;
update _build_tx_with_shielded_outputs to accept an optional timestamp parameter
(e.g., timestamp: int | None = None), set tx.timestamp = timestamp if provided
(otherwise keep current int(time.time())), and modify callers in the hash/UUID
inclusion tests to call both the plain and shielded builders with the same
pinned timestamp value so hashes/UUIDs are deterministic; apply the same pattern
to the other helper usages referenced (around the other occurrences noted) so
all paired builds share the same timestamp.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d9761df5-b420-4f49-b582-bf1495ab26ea
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Dockerfilepyproject.tomltests/test_shielded_outputs.py
✅ Files skipped from review due to trivial changes (1)
- pyproject.toml
Documents the manual build/verify/push procedure for the experimental hathornetwork/tx-mining-service:shielded-outputs-vN image, including the parent-dir build context, the hathorlib prerequisites (ShieldedOutputsHeader and UnshieldBalanceHeader), and the tag-bumping rule. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/docker-shielded-outputs.md (1)
23-25: Add language identifiers to fenced code blocks.The fences at Line 23, Line 32, Line 45, and Line 58 are missing language tags (MD040). Adding
bash/pythonimproves readability and keeps markdown lint clean.Suggested doc tweak
-``` +```bash python -c "from hathorlib.headers import ShieldedOutputsHeader, UnshieldBalanceHeader"-
+bash
cd /path/to/Hathor # parent of tx-mining-service and hathor-core-4
docker build
-f tx-mining-service/Dockerfile
-t tx-mining-service:shielded-outputs-v1
.-``` +```bash docker run --rm --entrypoint python tx-mining-service:shielded-outputs-v1 -c " from hathorlib.headers import ShieldedOutputsHeader, UnshieldBalanceHeader, VertexHeaderId from hathorlib.vertex_parser import VertexParser h = VertexParser.get_supported_headers() assert h[VertexHeaderId.SHIELDED_OUTPUTS_HEADER] is ShieldedOutputsHeader assert h[VertexHeaderId.UNSHIELD_BALANCE_HEADER] is UnshieldBalanceHeader print('ok') "-
+bash
docker tag tx-mining-service:shielded-outputs-v1
hathornetwork/tx-mining-service:shielded-outputs-v1
docker push hathornetwork/tx-mining-service:shielded-outputs-v1Also applies to: 32-38, 45-54, 58-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/docker-shielded-outputs.md` around lines 23 - 25, Add appropriate language identifiers (bash or python) to the fenced code blocks in docs/docker-shielded-outputs.md that are currently missing them (the blocks containing the python -c import line, the docker build block, the docker run -c block that runs VertexParser checks, and the docker tag/push block). Update each opening fence from ``` to ```bash or ```python as appropriate so lint MD040 is satisfied; for example mark the one-liner import as ```python and mark the docker build/run/tag blocks as ```bash. Ensure all four blocks referenced in the comment (around the import line and the three docker command blocks) are updated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/docker-shielded-outputs.md`:
- Around line 21-25: The host pre-check command can import a globally installed
hathorlib instead of the sibling checkout; update the check to force the
project's source path or virtualenv so the import of ShieldedOutputsHeader and
UnshieldBalanceHeader comes from the local ../hathor-core-4 checkout. Replace
the plain python -c invocation with one that sets PYTHONPATH to the sibling repo
(or activates the project venv) before running python -c "from hathorlib.headers
import ShieldedOutputsHeader, UnshieldBalanceHeader" so the correct local
package is validated.
---
Nitpick comments:
In `@docs/docker-shielded-outputs.md`:
- Around line 23-25: Add appropriate language identifiers (bash or python) to
the fenced code blocks in docs/docker-shielded-outputs.md that are currently
missing them (the blocks containing the python -c import line, the docker build
block, the docker run -c block that runs VertexParser checks, and the docker
tag/push block). Update each opening fence from ``` to ```bash or ```python as
appropriate so lint MD040 is satisfied; for example mark the one-liner import as
```python and mark the docker build/run/tag blocks as ```bash. Ensure all four
blocks referenced in the comment (around the import line and the three docker
command blocks) are updated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 95957b32-e4ff-4752-a004-438c1f888c51
📒 Files selected for processing (1)
docs/docker-shielded-outputs.md
| Quick check from the host before building: | ||
|
|
||
| ``` | ||
| python -c "from hathorlib.headers import ShieldedOutputsHeader, UnshieldBalanceHeader" | ||
| ``` |
There was a problem hiding this comment.
Host pre-check can validate the wrong hathorlib installation.
At Line 24, python -c ... may import a globally installed package instead of the sibling checkout, so this check can pass even when ../hathor-core-4/hathorlib is not the one being used. Consider forcing the source path (or using the project venv) in the command.
Suggested doc tweak
-```
-python -c "from hathorlib.headers import ShieldedOutputsHeader, UnshieldBalanceHeader"
-```
+```bash
+PYTHONPATH=../hathor-core-4 python -c "from hathorlib.headers import ShieldedOutputsHeader, UnshieldBalanceHeader"
+```📝 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.
| Quick check from the host before building: | |
| ``` | |
| python -c "from hathorlib.headers import ShieldedOutputsHeader, UnshieldBalanceHeader" | |
| ``` | |
| Quick check from the host before building: | |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 23-23: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/docker-shielded-outputs.md` around lines 21 - 25, The host pre-check
command can import a globally installed hathorlib instead of the sibling
checkout; update the check to force the project's source path or virtualenv so
the import of ShieldedOutputsHeader and UnshieldBalanceHeader comes from the
local ../hathor-core-4 checkout. Replace the plain python -c invocation with one
that sets PYTHONPATH to the sibling repo (or activates the project venv) before
running python -c "from hathorlib.headers import ShieldedOutputsHeader,
UnshieldBalanceHeader" so the correct local package is validated.
2 participants
Summary
hathorlibdependency from PyPI to a local path (../hathor-core/hathorlib) to pick up the newShieldedOutputsHeader,AmountShieldedOutput, andFullShieldedOutputtypesDockerfileto build from the parent directory so the local hathorlib source is available in the Docker build contextDependency
This PR expects
hathor-coreto be checked out at../hathor-core/on branchfeat/ct-amount-token-privacy, which contains the shielded output data structures and header serialization moved intohathorlib.Docker build
Since
hathorlibis now a local path dependency, the Docker build context must be the parent directory containing both repos:Test plan
tests/test_shielded_outputs.pypassfeat/ct-amount-token-privacy🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores
Documentation