-
Notifications
You must be signed in to change notification settings - Fork 1
Real blockchain transactions with Zaino/LWD backends and pexpect wallet integration with full M2 and zingolib integration #4
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.
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
base: main
Are you sure you want to change the base?
Conversation
Added healthcheck for zaino service and updated dependencies.
pacu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First review.
I haven't set up a development environment for this yet so I haven't run the code. Yet I identified certain things that don't work correctly or at all.
cli/Cargo.toml
Outdated
| @@ -0,0 +1,40 @@ | |||
| [package] | |||
| name = "zecdev" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Project is called ZecKit but the CLI is called zecdev. Given that ZecDev is a GitHub Org dedicated to development sources. Please call this tool ZecKit as ZCG indicated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad would update this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this file supposed to be checked in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope at All it's not useful would remove
| echo "📊 Current block height: ${BLOCK_COUNT}" | ||
|
|
||
| # Wait for blocks | ||
| echo "⏳ Waiting for at least 10 blocks to be mined..." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe this is actually necessary. LWD should respond accordingly to a syncing or initializing node. I think that this actually creates an artificial setup that might mask out some problems that are worth looking into if they happen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay was waiting cuz of It doesn't sync immediately so was like a workaround
faucet/app/routes/faucet.py
Outdated
| faucet_bp = Blueprint('faucet', __name__) | ||
|
|
||
|
|
||
| def validate_address(address: str) -> tuple: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is insufficient to validate a Zcash address. It defeats the purpose of the tool to actually have these kind of things. Also, there's no consideration whatsoever for the regtest network which this dev tool uses
use the full node validate address RPC or implement address validation via FFI with librustzcash tools.
use the test vectors to validate your results
https://github.com/zcash/zcash-test-vectors/tree/master/test-vectors/zcash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm gonna rebuild the faucet in Rust. That way I can use librustzcash directly, validate against the actual test vectors, and handle regtest addresses properly. Plus it'll integrate way better with the Zebra node we're already running.
Thanks for calling that out 🙏
faucet/app/wallet.py
Outdated
|
|
||
| if isinstance(result, dict) and 'output' in result: | ||
| output = result['output'] | ||
| match = re.search(r'uregtest1[a-z0-9]{70,}', output) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all of this matching can break by a change upstream. It's very unstable. There should be tests that actually cover for this kind of issues.
The flask faucet is actually introducing this room for flukes and errors. Probably the Faucet should be coded on Rust to actually use ZingoLib as a library and avoid these issues all along
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm rebuilding the faucet in Rust. Direct ZingoLib integration, proper error handling, no fragile string parsing. Tests covering everything.
Should've done it that way from the start.
scripts/mine-regtest.py
Outdated
|
|
||
| for i in range(n): | ||
| current = mine_block() | ||
| if current is False or current == start_count + i: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this succeeds if the first mine_block call succeeds. Why is so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic is broken. It should check that each block actually mined successfully before moving to the next one. Right now it just passes if the first one works, doesn't actually verify all n blocks got mined.
I'll fix it to properly validate each block in the sequence.
scripts/mine-regtest.py
Outdated
| for i in range(n): | ||
| current = mine_block() | ||
| if current is False or current == start_count + i: | ||
| print(f"✅ Mined block {i+1}/{n} (height: {current})") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haven't tested yet but this appears to be wrong because when you mined 10 blocks, and call this again, this message will say "i✅ Mined block 1 (height: 12)" because L45 already mined a block and you are mining one more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah you're right. The loop counter doesn't match the actual block height. It says "block 1" but you're really on block 12.
Should just show the actual height instead of the loop number.
scripts/mine-regtest.py
Outdated
| else: | ||
| print(f"⚠️ Block {i+1}/{n} might have failed") | ||
|
|
||
| final_count = mine_block() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
paper test for n=5
n=5
"🔨 Mining 5 blocks..."
start_count=1 <<<=== this mines a block
For i in range(n)
// i = 0
"✅ Mined block {1}/{5} (height: 2)"
// i = 1
"✅ Mined block {2}/{5} (height: 3)"
// i = 2
"✅ Mined block {3}/{5} (height: 5)"
// i = 3
"✅ Mined block {4}/{5} (height: 6)"
// i = 4
"✅ Mined block {5}/{5} (height: 6)"
final_count = 7 <<<== this mines another block
"🎉 Final block count: 7"
"📊 Mined 6 blocks"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're mining 7 blocks total but saying you mined 6. The logic is all off the initial mine_block() call shouldn't be there, and final_count shouldn't mine another block. Should just loop n times, mine each block, track the actual count. No extra calls. I'll fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be README.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad wanted to document the Cli would make change's
confirm
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the effect of this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was meant to run a locally Ci runner so you can see whats going on in the Code like tests that are happening
|
Thanks @pacu , really appreciate it. I'm gonna fix some faucet issues wanted to use python for quick prototyping might write everything in Rust or fix the issues in python , fix those mining script issues, and do it properly this time. If something breaks or isn't working, I'll hit you up instead of trying to patch it with workarounds. No shortcuts - we'll actually fix the root issues. |
|
Made changes requested @pacu thank you would await reviews |
|
Hi @Timi16 you have requested a re-review of this PR but I don't see any other changes. |
|
So i decided to change the faucet from Python to Rust so i can interact with Zingolub directly |
pacu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
partial review.
not tested yet. there are some naming issues remaining. Also rust faucet should use librustzcash instead ofZebra RPC to validate addresses. Also left questions to the developer about method visibility on Axum handlers
pacu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've finished reviewing the code but I haven't tested it yet.
Next steps: @pacu will test the tool in his development server to verify the functionality of the code and point out any issues encountered with the features planned for milestone 2 if any
Milestone 2: Real Blockchain Transactions
This PR delivers M2 core functionality: a working Zcash regtest devnet with real on-chain transactions, dual backend support (Lightwalletd + Zaino), and a functional faucet API using pexpect for reliable wallet interaction.
✅ What's Delivered
Core Infrastructure:
Real Blockchain Transactions:
Developer Experience:
zecdev) with up/down/test commandsKey Technical Achievements:
📊 Test Results
Zaino Backend:
Real Transaction Proof:
🔧 Technical Highlights
Pexpect Implementation:
Healthcheck Optimization:
Backend Toggle:
📚 Documentation Added
README.md- Complete user guide with quickstart (updated)specs/technical-spec.md- 27-page implementation deep-dive (NEW)specs/architecture.md- System design and data flows (NEW)scripts/setup-mining-address.sh- Mining address configuration (NEW)🐛 Bugs Reported Upstream
🚀 What This Enables
📋 Files Changed
New Files:
specs/technical-spec.mdspecs/architecture.mdscripts/setup-mining-address.shdocker/lightwalletd/Dockerfile(Go 1.24 build)docker/lightwalletd/entrypoint.sh(healthcheck logic)Modified Files:
README.md(comprehensive rewrite)docker-compose.yml(healthcheck tuning, backend profiles)faucet/app/wallet.py(pexpect implementation)faucet/app/main.py(startup optimization)docker/configs/zebra.toml(mining configuration)✅ M2 Acceptance Criteria
From Grant:
Status:
🙏 Acknowledgments
Thanks to the Zcash community for:
Ready for review! Infrastructure is solid, real transactions working.