re: #29015 (review)
I was mostly thinking that it doesn’t make sense for TransactionErrorString() in common even though it depends on TransactionError which is in node, and node depends on common, not vice-versa? I wasn’t expecting paragraphs of response :)
Yeah sorry thanks for pinning down the actual concern. Before you just said it “seems odd” so I felt like I needed to justify my entire thought process.
In any case, both the node and wallet libraries expose public types which I listed above (CNodeStats, CNodeStateStats, and SynchronizationState, AddressPurpose, isminetype, CRecipient, and CCoinControl) which can be considered part of the interfaces to these libraries, but are not part of the libraries themselves. For example, the GUI code is not allowed to call node or wallet functions, but it uses all of these public types, and this is fine. Other code, including code in common, can also use these types and there is no problem with this.
We do have a choice where about where to put public types which are not really part of any library. We can put them in the interfaces directory, or in common, or in the node and wallet directories. Personally, I think it makes sense to put them in the wallet directory when they are related to wallet functionality and used by wallet code, put them in the node directory when they are related to node functionality and used by node code, put them in the common directory when they are related to common functionality (like PSBT) and used by common code. The comments at the top of {node,wallet,common}/types.h files are meant to explain the idea that these are public types.
Doesn’t really seem worth worrying about the fact that some of the errors only make sense in a node context; aren’t these all just “something went wrong, deal with it, dear user”?
Yes if errors are just being propagated to the user level, you don’t care about this at all, and it’s probably a mistake to use an enum to begin with because the error messages won’t be able to contain useful context. In this case, though, the errors are being translated to RPC codes, and RPC callers may actually want to implement different forms of error handling, so it would be good to be clear about what errors are possible, plus it’s easy to do, so I did add a new commit splitting PSBTError from TransactionError.
BTW, I think TransactionError::P2P_DISABLED can be dropped entirely? (I think the last/only use was removed in #15713)
Thanks, the new commit drops this code too.
