Remove Safe mode (achow101) #13090

1. 
   laanwj commented at 1:34 pm on April 26, 2018: member

   Rebase of #10563. Safe mode was disabled by default and deprecated in 0.16, so probably should be removed for 0.17.

   Rationale:

   Safe mode is useless. It only disables some RPC commands when large work forks are detected. Nothing else is affected by safe mode. It seems that very few people would be affected by safe mode. The people who use Core as a wallet are primarily using it through the GUI, which safe mode does not effect. In the GUI, transactions will still be made as normal; only a warning is displayed.

   I also don’t think that we should be disabling RPC commands or any functionality in general. If we do, it should be done consistently, which safe mode is not. If we want to keep the idea of a safe mode around, I think that the current system needs to go first before a new system can be implemented.

2. Remove Safe mode 2ae705d841
3. laanwj added the label RPC/REST/ZMQ on Apr 26, 2018
4. laanwj added this to the milestone 0.17.0 on Apr 26, 2018
5. MarcoFalke renamed this:
   Remove Safe mode (rebased)
   Remove Safe mode (achow101)
   on Apr 26, 2018
6. 
   MarcoFalke commented at 1:36 pm on April 26, 2018: member

   Is there an issue open to remind us of implementing something better? If not, that should probably done before merge.
7. 
   fanquake commented at 1:39 pm on April 26, 2018: member

   @MarcoFalke The “something better” is likely to be hashed out in the comments here, an issue can probably be opened after that.
8. 
   laanwj commented at 1:40 pm on April 26, 2018: member

   I’m not sure we need “something better” for safe mode. We need something better for fork/isolation detection (e.g. improve CheckForkWarningConditions), which would generate a warning, but I don’t think safe mode (as in, automatically disabling RPC calls) needs to be brought back.
9. 
   achow101 commented at 2:38 pm on April 26, 2018: member

   The rebase looks right, utACK 2ae705d84178fb9faa49f92091206e92379a2c63
10. 
    practicalswift commented at 3:21 pm on April 26, 2018: contributor

    Concept ACK
11. 
    jtimon commented at 8:24 pm on April 26, 2018: contributor

    utACK 2ae705d84178fb9faa49f92091206e92379a2c63
12. 
    sipa commented at 8:40 pm on April 26, 2018: member

    utACK 2ae705d84178fb9faa49f92091206e92379a2c63
13. 
    MarcoFalke commented at 9:20 pm on April 26, 2018: member

    Please remove all code related to this, specifically:

    • Leftover RPC_FORBIDDEN_BY_SAFE_MODE
    • Usage of "rpc" in the context of warnings

    utACK 2ae705d84178fb9faa49f92091206e92379a2c63 otherwise

14. 
    laanwj commented at 10:04 pm on April 26, 2018: member

    Leftover RPC_FORBIDDEN_BY_SAFE_MODE

    I thought about this, but decided to keep it - we wouldn’t want to accidentally re-use the number. Could replace it with a comment that the number is reserved, I guess… (but as they’re not sorted by number, it’s a risk)

    Edit: also some API documentation / bindings are directly generated from the .h file - this is why the “Aliases for backward compatibility” are there. We don’t want to break code that relies on this error code existing, even if it’s never generated.

    Usage of “rpc” in the context of warnings

    Ok, will remove that.

15. 
    MarcoFalke commented at 10:44 pm on April 26, 2018: member

    Then, it seems adjusting the comment slightly will help for future reference. Otherwise some linter will come around and remove it ;)
16. 
    laanwj commented at 7:01 am on April 27, 2018: member

    Then, it seems adjusting the comment slightly will help for future reference. Otherwise some linter will come around and remove it ;)

    Yes - I think I’ll move it to a ‘reserved’ section.

17. rpc: Move RPC_FORBIDDEN_BY_SAFE_MODE code to reserved section

    Although this code is no longer ever sent back after removing safe mode,
    it would be unwise to remove it from the header.
    
    For one, it would be bad to accidentally re-use the number.
    
    Also some API documentation / bindings are directly generated from the .h
    file - this is why the "Aliases for backward compatibility" are there. We don't
    want to break code that relies on this error code existing, even if it's never
    generated.
    
    So keep it around but move it to a reserved section.

    7da3b0adb8
18. Remove "rpc" category from GetWarnings

    No longer used after removing safe mode.
    
    This function can likely be simplified more, but I'll leave that
    for later to make this easy to review.

    d8e9a2ac74
19. 
    laanwj commented at 7:32 am on April 27, 2018: member

    Added commits:

    rpc: Move RPC_FORBIDDEN_BY_SAFE_MODE code to reserved section

    Although this code is no longer ever sent back after removing safe mode, it would be unwise to remove it from the header.

    For one, it would be bad to accidentally re-use the number.

    Also some API documentation / bindings are directly generated from the .h file - this is why the “Aliases for backward compatibility” are there. We don’t want to break code that relies on this error code existing, even if it’s never generated.

    So keep it around but move it to a reserved section.

    Remove “rpc” category from GetWarnings

    No longer used after removing safe mode.

    This function can likely be simplified more, but I’ll leave that for later to make this easy to review.

20. 
    MarcoFalke commented at 12:18 pm on April 27, 2018: member

    Thanks. utACK d8e9a2a
21. 
    practicalswift commented at 12:43 pm on April 27, 2018: contributor

    utACK d8e9a2ac74e1071c9a92b9858b7ed3143413ee94

    Nice cleanup!

22. 
    promag commented at 2:23 pm on April 27, 2018: member

    utACK d8e9a2a.

    I guess it doesn’t make sense to add assert(code != RPC_FORBIDDEN_BY_SAFE_MODE) to JSONRPCError.

23. 
    practicalswift commented at 2:31 pm on April 27, 2018: contributor

    @promag Why wouldn’t that make sense? Explicit is better than implicit :-)

    +1 for adding assert(code != RPC_FORBIDDEN_BY_SAFE_MODE) to JSONRPCError

24. 
    laanwj commented at 2:36 pm on April 27, 2018: member

    I’m not going to add that assert. In the unlikely case that that happens, returning that error code would be preferable to crashing the whole process with an assert error. It also feels like overdesign to add a consistency check for this.
25. 
    practicalswift commented at 2:49 pm on April 27, 2018: contributor

    @laanwj I see your point w.r.t. possible impact of returning the now obsolete error code. You made me change my mind:

    -1 for adding assert(code != RPC_FORBIDDEN_BY_SAFE_MODE) to JSONRPCError

    :-)

26. 
    MarcoFalke commented at 2:58 pm on April 27, 2018: member

    Clearly, an assert in rpc code makes no sense… I think this is ready for merge, since we reached our final destination, the bike shed.
27. laanwj merged this on Apr 27, 2018
28. laanwj closed this on Apr 27, 2018

    ---

29. laanwj referenced this in commit 17266a1306 on Apr 27, 2018
30. PastaPastaPasta referenced this in commit bfb100bdd8 on Jun 17, 2020
31. PastaPastaPasta referenced this in commit 0ab2f79f53 on Jun 27, 2020
32. MarcoFalke locked this on Sep 8, 2021

Contributors
laanwj MarcoFalke fanquake achow101 practicalswift jtimon sipa promag

Labels
RPC/REST/ZMQ

Milestone
0.17.0