Tx, Block, VM: Consistent Error Context for Error Messages #1540
Conversation
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
holgerd77
force-pushed
the
client-block-error-and-debug-improvements
branch
2 times, most recently
from
20ed41f to
d63d8d7
Compare
|
Update: after some more thinking about this I have gotten a bit more conservative here and now have some stronger tendency to go with variant 3 and do a dedicated flag. Then we are on the safe side here and can really expand throughout the library set without risking side effects. Will likely work on this throughout large parts of next week, I guess if this is crafted with some care it will substantially help us to safe on debugging time in the future. |
|
Any opinions/preferences here on/for 1, 2 or 3? 🙂 |
packages/block/src/header.ts
Outdated
| let msg = `block header number=${this.number} hash=${bufferToHex(this.hash())} ` | ||
| msg += `hf=${this._common.hardfork()} baseFeePerGas=${this.baseFeePerGas ?? 'none'}` |
There was a problem hiding this comment.
Why not define this in a single const?
There was a problem hiding this comment.
Thanks for giving this some look! 😄
Sorry, I have the habit to separate too long string lines this way, is this bad practice? 😋 Not sure, would the linter do such things properly in some way otherwise? Will give this a try.
There was a problem hiding this comment.
Hmm, the linter is doing something like the follow out of this:
const errorPostfix = `block header number=${this.number} hash=${bufferToHex(
this.hash()
)} hf=${this._common.hardfork()} baseFeePerGas=${this.baseFeePerGas ?? 'none'}`Guess I don't like this either, don't find it very readable, will stick with my original version for now.
I don't have a clear view of the potential impact of releasing this as is for users, but my feeling would be to go for 2, and perhaps re-consider 3 if the pre-announcement results in substantial pushback? |
|
Concerning the error management that we currently have in the works (see #1469, which is ready for review), I think such an error management system would fit right in this package (block), implementing what the |
|
@gabrocheleau Ok, thanks 🙂, I am still a bit unsure if 2. might end up causing too much hazzle for users, on the other hand from a cleanliness perspective I would also prefer to do this straight without a flag. Eventually @ryanio or others can also have a word here. Eventually even @alcuadrado find some short time to drop a note from an outside perspective. I would then continue to do the updates here on the other libraries. (I've put a more complex solution for error management on hold for now, see comment along #1469, think this overloads us atm and on the other side we can really need some imminent improvements on this right now) |
…on in error case, made _error() protected (do not call from the outside but keep extensible), more robust error case hash creation
holgerd77
force-pushed
the
client-block-error-and-debug-improvements
branch
from
d63d8d7 to
8061d72
Compare
…ror annotation for cleaner stack trace
holgerd77
force-pushed
the
client-block-error-and-debug-improvements
branch
from
b2213d9 to
58f55f3
Compare
…compact error representation of the object, adding consistent error context to VM.runTx()
holgerd77
force-pushed
the
client-block-error-and-debug-improvements
branch
2 times, most recently
from
79ca83f to
130b59b
Compare
holgerd77
force-pushed
the
client-block-error-and-debug-improvements
branch
from
130b59b to
14cdd66
Compare
holgerd77
changed the title
Ok, after now being annoyed about this 1000 times or so I've finally done it and added a consistent error message append to all block error messages thrown. This should help us and also others a great deal on debugging, I've now added additional
console.logoutput countless times in the client on errors like described in #1539 (4)invalid tx trieto just find out in the first place what block is even touched and to what HF this block is set.I know that we had this discussion about if this would be a breaking change or not. In my current thinking the benefits of having these substantially better debugging capabilities greatly outweight the risk of having this break for 2-3 people who are a) doing specifc error comparison at all and b) are not doing it the recommended way using
includes()or similar.We can nevertheless do different approaches here.
extendedErrorMsgs = falsewhich will be removed on the next breaking releaseI guess even with 3. this would be worth it, then we could at least use this flag at the 4-5 instantiation places in the client e.g. already, somewhat more of a hazzle and coordination effort though.
I would generally have a tendency to expand on this for all the other libraries (or let's say: VM, Blockchain, Tx) implementing in the way we agree upon (so: 1,2 or 3).
(and also: I know we have got some error management rework in the works but this a) still seems to be too far away atm to be imminently useful and b) is not clear to affect the libraries most in need on first round (block, tx).)