Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Blockchain debug additions #3676

Merged
merged 14 commits into from
Sep 19, 2024
Merged

Blockchain debug additions #3676

merged 14 commits into from
Sep 19, 2024

Conversation

scorbajio
Copy link
Contributor

This change adds debug logging to core parts of the blockchain package as discussed in #3671

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks for starting, but this is not yet going as deep as I intended with the issue! We can eventually nevertheless merge when the comments are addressed, since it is for sure already consistent and useful on its own.

I meant this more than a simple work down issue though and really go one step deeper and give this some substantial thinking where debug messages are useful, what are delicate situations and so on.

Not sure, maybe I am also off here myself, wait, I will just go through the library and write down what I would identify as "interesting and/or debug-worthy events" 🙂 (might mention some which are already in the logs, won't "sync" with the changes here, then simply ignore).

Will number here, just if we want to "talk about one of them", so order has no other meaning:

  1. When a genesis block is initialized (so that's in the constructors)
  2. How many blocks are put with putBlocks or putHeaders
  3. How many blocks are deleted with _deleteCanonicalChainReferences
  4. From where to where resetCanonicalHead is setting a head
  5. What common ancestor findCommonAncestor has found and how many ancestor headers
  6. How many staleHeads have been found (also citing header) in _rebuildCanonical

Ok. That's what I could identify. 🙂

I guess that's not too much! Can you therefore directly in this PR?

| ----------------------------- | ------------------------------------------------------------------------ |
| `blockchain:core` | Core blockchain operations like when a block or header is put or deleted |
| `blockchain:consensus:clique` | Clique consensus operations like updating the vote and/or signer list |
| `blockchain:consensus:ethash` | Ethash consensus operations like PoW block or header validation |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we really please keep these logger names short (and not over-namespacify 😂 )? We do not need to be 100% consistent here and not sure if you have used loggers a lot in the past but it really becomes annoying quickly if these logger names are long.

(also this :core addition is a bit inconsistent, since we are not doing/using this on any other logger for the core class)

So let's please simply do blockchain, blockchain:clique and blockchain:ethash. This is mostly for our internal usage and we can make "breaking" changes here anytime.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed this back to what it was before for now. Will create an issue on rethinking namespaces to allow wildcards like DEBUG=ethjs,blockchain:* work for all namespaces in blockchain and not unexpectedly exclude the core namespace, as is happening currently.

packages/blockchain/src/blockchain.ts Outdated Show resolved Hide resolved
Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of these debug messages can need a bit more content. Will push an update, then we can merge! 🙂

@holgerd77
Copy link
Member

Ok, pushed some updates, not so easy admittedly. Guess it's still worthy to clean-up blockchain, there some room for small improvements and clarifications on a lot of fronts and it's generally still somewhat hard to grasp the context of things and follow the code.

Anyhow: Debug logging now looks like this (taken from test run), definitely a start. We can go from there everytime we work with blockchain. Guess there is still some adjustments to be made (some logs too verbose, some missing):

grafik

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@holgerd77 holgerd77 merged commit 4aa4cef into master Sep 19, 2024
39 checks passed
@holgerd77 holgerd77 deleted the blockchain-debug-additions branch September 19, 2024 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants