ethereum / go-ethereum

Go implementation of the Ethereum protocol
https://geth.ethereum.org
GNU Lesser General Public License v3.0
47.54k stars 20.13k forks source link

FR: Separate Dangerous Debugs From Useful Debugs #21963

Open MysticRyuujin opened 3 years ago

MysticRyuujin commented 3 years ago

Rationale

The debug_ namespace of the JSON RPC server has some really useful functions available to end user, however, it also has some very dangerous functions in the same namespace. It's impossible to turn on the useful features without also turning on the dangerous ones.

Things like debug_traceTransaction should probably not be in the same namespace as debug_setHead

Alternatively, a useful feature that Nethermind and Tubo-Geth have is a JSON RPC filter, where we can selectively enable or disable specific functions instead of exposing all function under a given namespace.

Implementation

Do you have ideas regarding the implementation of this feature? I just see the 2 paths listed above as options, move dangerous functions to their own namespace, or let us create a filter, which might be useful for all of the available namespaces.

Are you willing to implement this feature? I would if I had any idea what I was doing and was a competent Go programmer, but I am not :smile:

ligi commented 3 years ago

Moving might be problematic as this is a breaking change. Also something like this should be coordinated between clients (so Ideally start a thread on eth-magicians and/or an EIP) So I think the only short-term solution is a filter. Just wondering if it should be part of geth or for the sake of reusability/modularity a proxy in front of geth.

MysticRyuujin commented 3 years ago

I would agree with the EIP thing if the debug namespace was part of any EIPs, I'm not sure they are. Parity/OE only has debug_getBadBlocks because Martin asked them to implement it.

I'm usually the first one to complain about RPC stuff not being standardized lol

My objective/goal here is to be able to safely expose these RPC endpoints on a node, directly (without a proxy), without the risk of accidentally exposing something dangerous that would affect the database of an Archive Node.

Even turning them on with a proxy in front of it (which we do have now) is still worrying.

I'm perfectly happy with a filter.

fjl commented 3 years ago

I'm also not happy about some of those functions, especially the ones that write files. We should move those to a separate namespace.

I think the method filter is not the best solution. It works, but the APIs should really be structured properly.

holiman commented 3 years ago

@MysticRyuujin in the case of debug_traceTransaction, isn't that one of the methods that you would like to serve to your user, as part of the non-dangerous set? Because if so, then it doesn't really solve anything if we tag it as 'dangerous'.

I think the root problem for that is that we need to limit the amount of data that we shove back over RPC. Normally, a crash in rpc-handing is 'localized' to the goroutine, and doesn't cause a geth-global crash. But OOM is one of the exceptions.

MysticRyuujin commented 3 years ago

My linking to this issue is not in response to that call being dangerous, just linking issues because people who care about these RPC methods probably care about this as well.

I don't see these as independent issues, I view it as a systemic design issue with the debug namespace as a whole.