-
Notifications
You must be signed in to change notification settings - Fork 60
v2 -- Bitcoin Core Implementation #207
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
This PR has been automatically marked as stale because it has been open 30 days |
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.
Thanks again for the contribution! It is a great blueprint and it will be wonderful to have it merged once all the problems are fixed.
Please also note that you need to add a file with reference to the local README.md
inside the website/docs/blueprints
directory to make it visible on the public web page. Example: https://github.com/aws-samples/aws-blockchain-node-runners/blob/main/website/docs/Blueprints/Base.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.
This is a command line script and uses generic "crypto" module. Choosing to use JavaScript for this script introduces extra dependencies to the global OR local package.json
files, which is not allowed.
Can it be rewritten in bash with standard linux tools and OpenSSL for crypto?
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 understand that "introducing extra dependencies to the global or local package.json
is not allowed", and it is a personal opinion you have.
However, it is absolutely necessary to incorporate the Secrets Manager dependency, as the generateRPCAuth script requires it to securely store the credentials which are needed to access the node. (To my understanding, this is the only Blueprint that requires credentials for performing external RPC Requests )
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.
FYI, the generateRPCAuth.js
script is based on an opensource tool.
The above links to this script , which I expanded upon.
Though it can be rewritten in bash, I do not believe it is worth my time or effort to do so.
I am closing 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.
We have successfully maintained our protocol of avoiding additional dependencies in both global and local package.json files across all blueprints. Please continue adhering to this standard by:
- Evaluating if new dependencies are absolutely essential, rather than merely convenient
- Prioritizing Bash implementations where feasible
- Thoroughly exploring alternative solutions before introducing new dependencies
Please note that this requirement serves as a critical blocking issue for this blueprint. The inclusion of new dependencies will only be considered if there is substantial evidence demonstrating that:
- A Bash implementation is not viable
- Alternative dependency-free solutions are not feasible
Before proceeding, please ensure these criteria are thoroughly evaluated and documented.
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.
Please note that crypto
module is part of NodeJS standard API. There shouldn't be any addition to package.json
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.
Noted with thanks. The script includes "@aws-sdk/client-secrets-manager" and "base64url" that are also added as dependencies to the root package.json file, and that is a problem. See our conversation here: https://github.com/aws-samples/aws-blockchain-node-runners/pull/207/files/c78aab85e13997f02078bfedd442835b22ec098c#r2244133508
The reason I brought up "crypto" in the original message is to highlight that there is no Bitcoin-specific cryptography involved in this logic. Therefore, there is a potential to use OpenSSL to do the same in Bash. This needs to be researched.
const { SecretsManagerClient, CreateSecretCommand, PutSecretValueCommand } = require('@aws-sdk/client-secrets-manager'); | ||
|
||
// Set up AWS SDK client | ||
const client = new SecretsManagerClient({ region: 'us-east-1' }); // Change region if needed |
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.
AWS region is hard coded. Please make this configurable as the script can be used in different regions.
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.
Please follow the sync-checker
pattern for script naming and use of systemd services instead of corn. Cron is no longer comes as standard in newer versions of Linux. Example implementation of SyncChecker scripts: https://github.com/aws-samples/aws-blockchain-node-runners/tree/main/lib/base/lib/assets/sync-checker
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.
Please make sure the script reports at least two metrics: Current Block Height
and Blocks Behind
. That way operator will get a clearer picture on how the node is performing.
"mem": { | ||
"measurement": ["mem_used_percent"] | ||
}, | ||
"cpu": { |
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.
Please add "cpu_usage_iowait"
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 file name says it is a configuration for mainet
, but the config doesn't seem to have a variable setting to choose between mainnet
and testnet
. Is it possible to configure in bitcoin? And, if yes, can it be implemented in the blueprint?
template.hasResourceProperties("AWS::ElasticLoadBalancingV2::TargetGroup", { | ||
Port: 8332, | ||
Protocol: "HTTP", | ||
}); |
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 test doesn't validate Security Groups and actual configuration parameters for Launch Templates (just matches any value
). Also skips testing majority of the parameters for ALB target group, ALB configuration, autoScaling Group ( just matches any value
), Instance IAM role, etc. Please make testes more explicit and follow this example for reference:
template.hasResourceProperties("AWS::EC2::SecurityGroup", { |
|
||
// Has EC2 instance with node configuration | ||
template.hasResourceProperties("AWS::EC2::Instance", { | ||
InstanceType: Match.stringLikeRegexp(".*"), // accept any value including 'undefined.undefined' |
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.
Please verify instance type
|
||
|
||
// Has CloudWatch dashboard | ||
template.hasResourceProperties("AWS::CloudWatch::Dashboard", { |
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.
Please validate that dashboard name follows naming convention as it affects usability. Example:
DashboardName: { |
npx cdk deploy HABitcoinCoreNodeStack --json --outputs-file ha-nodes-deploy.json | ||
``` | ||
|
||
### Deployment Architectures for Bitcoin Nodes |
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.
Please move this introductory section with architecture overview to the top, before Well Architected.
What does this PR do?
🛑 Please open an issue first to discuss any significant work and flesh out details/direction - we would hate for your time to be wasted.
Consult the CONTRIBUTING guide for submitting pull-requests.
Motivation
License
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
More
README.md
file in my blueprint folderREADME.md
file towebsite/docs
section for this featureFor Moderators
cdk-nag
tools don't show warnings?README.md
before merge?Additional Notes
I have incorporated all feedback to refactor the Blueprint to better align with other ones.