-
Notifications
You must be signed in to change notification settings - Fork 102
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
add(Docker): Docs for mining with Docker #7179
Conversation
We'll need to test the commands in the docs when we do the next release. It would be good if we could test them before the release, but I don't know how. |
You can test the commands on the release image in the CD builds, they use the
It seems like you'll want to test against But it doesn't use the zebra/.github/workflows/continous-delivery.yml Lines 16 to 22 in f6afec2
zebra/.github/workflows/continous-delivery.yml Lines 109 to 115 in f6afec2
Or we could add a manual release trigger and tag it with the branch name. But that seems riskier, we could accidentally publish something that looks like a real release, and users would use it. (It might also update the |
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.
LGTM!
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.
Have these commands been tested against the Docker image changes yet?
You can build Docker images locally if that helps?
I tested this locally, as a result of executing the commands in this PR I got these configurations: Running Creates this [network]
network = "Testnet"
listen_addr = "0.0.0.0"
[state]
cache_dir = "/var/cache/zebrad-cache"
[tracing]
use_color = false
[mining]
miner_address = "t27eWDgjFYJGVXmzrXeVjnb5J3uXDM9xH9v" Running Creates this [network]
network = "Mainnet"
listen_addr = "0.0.0.0"
[state]
cache_dir = "/var/cache/zebrad-cache"
[rpc]
listen_addr = "0.0.0.0:8232"
[tracing]
use_color = false
[mining]
miner_address = "t1XhG6pT9xRqRQn3BHP7heUou1RuYrbcrCc" |
It looks like a RPC address is missing from this config, maybe it needs some parts of PR #7175? Or the |
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.
Content looks good to me. I appreciate the clear examples.
I have not run any of the commands so I don't know whether they work or not which is why I'm not explicitly approving this PR.
I think testing the commands locally doesn't suffice. We need to test them against the images we publish.
Yes, this is also true for the currently published images. #7175 attempts to fix it. I tried describing what the issue is and why the port doesn't propagate to Zebra's config in the PR description. I don't see how we could leverage the |
I set #7175 as a blocker for this PR. Let's figure out a proper config for the images, and then test this PR? |
After speaking with Marek, these instructions won't work until we publish the next release so I'm blocking this on #7136 |
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.
Minimum changes to get it in
Did we do the required tests to ensure that these instructions will work? |
@mpguerra With the required changes from my review, it's working. |
I didn't, and we ended up with images with Mainnet enabled by default which isn't what we intended. However, after thinking about it, I think this change is actually suitable because there will be no API change when we start officially supporting mining, and I don't really see how having Testnet enabled by default protects from wasting the miner's resources, which is the risk we wanted to avoid. The reason is that to waste resources, the miner has to use a significant mining power for an extended period of time, and by that time, they must know whether they're on Mainnet or Testnet. |
@upbqdn when I tested this the first time I used this command, which defaults to docker run -e MINER_ADDRESS="t27eWDgjFYJGVXmzrXeVjnb5J3uXDM9xH9v" -p 18232:18232 zfnd/zebra:v1.1.0.experimental And the application panicked with the following error: The application panicked (crashed).
Message: assertion failed: `(left == right)`
left: `Testnet`,
right: `Mainnet`: incorrect miner address config: t27eWDgjFYJGVXmzrXeVjnb5J3uXDM9xH9v network.network Mainnet and miner address network Testnet must match
Location: /opt/zebrad/zebra-rpc/src/server.rs:158 From my perspective, I doubt the user will be able to use the wrong Network, but I might not be considering other use-cases. |
The issue isn't related to using an incompatible address; Zebra will always catch that. Our concern was that since we didn't have the means to test some of the mining functionality on Mainnet, users might run into bugs when running the image on Mainnet out of the box. However, as you pointed out, the image won't even start out of the box. Users have to first provide an address, which is either for Mainnet or Testnet, so they have to implicitly pick the network anyway. It's just a bit less of a hassle to start on Mainnet now, which I think is good. |
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.
Tested and LGTM!
I refactored almost all of the docs. |
The examples work for me as well. |
The issues raised in the review were addressed.
Motivation
Close #6611.
Depends-On: #7200 and #7231.
Solution
Reviewer Checklist
Follow Up Work
#7008.