goal: catchup without args prompts to continue#5165
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5165 +/- ##
==========================================
- Coverage 53.58% 53.53% -0.05%
==========================================
Files 439 439
Lines 54934 54942 +8
==========================================
- Hits 29438 29415 -23
- Misses 23212 23233 +21
- Partials 2284 2294 +10
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Co-authored-by: Shant Karakashian <55754073+algonautshant@users.noreply.github.com>
algorandskiy
force-pushed
the
pavel/goal-catchup
branch
from
dcb9125 to
6e4b099
Compare
winder
left a comment
winder
left a comment
There was a problem hiding this comment.
Unfortunately, this change would break a lot of automation. At the very least, it breaks our new docker container and sandbox. I don't think it's reasonable to change the behavior at this point. My recommendation is to update the description to be clearer about the security implications.
Searching github, there are 500+ matches and they would all break as well:
https://github.com/search?q=%22goal+node+catchup%22&type=code
I checked first 4 pages and they all use explicit argument so no prompt for them.
this is very easy to fix: |
|
imo the current state of things create more problems than fixing automation in couple places. |
|
I think you are simplifying the severity of this situation by simply asking for Even if someone is generating and using their own catchpoints, they should clearly understand that the security of that catchpoint handling/storage should be at the same level as their private keys. Yes, one node's compromised catchpoint will not be as damaging to them as loosing their private keys, but the moment the catchpoint vulnerability crosses the threshold, all bets of off. And just to put this risk into perspective, the blockchain security is distributed, but if the majority of the stake uses the same cloud storage, then the security is systemic/single point of failure. |
|
This is similar to the behavior of another goal command |
|
Applied fixes as requested |
|
Could you add this warning to the command description as well? |
done |
| if !fastCatchupForce { | ||
| fmt.Printf(nodeConfirmImplicitCatchpoint, label) | ||
| reader := bufio.NewReader(os.Stdin) | ||
| text, _ := reader.ReadString('\n') | ||
| text = strings.Replace(text, "\n", "", -1) | ||
| if text != "yes" { | ||
| reportErrorf(errorAbortedPerUserRequest) |
There was a problem hiding this comment.
would this be worth being a utility function for "get user input"?
There was a problem hiding this comment.
In the cmd directory we call ReadString in 5 places, there's probably some duplicate code there. Sounds like a fun little refactor, you should make a PR.
8 participants
Summary
Current
goal node catchupwithout args command starts a fast catchup process without making sure user understands that happens.This might give an illusion about security guarantees by assuming it is the same as downloading blocks and validating them.
Suggested fix
goal node catchupwithout args prompts user about possible security risk by using possible untrusted catchpoint and/or peers.goal node catchup --forcebypasses the prompt.No changes to
goal node catchup catchpoint-labelTest Plan
Tested manually