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

node, node-data: truncate trace logs of contract deployments #3025

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

d-sonuga
Copy link
Collaborator

To resolve the noisy log as described in #2742, the ContractDeploy::bytecode field now pretty prints as ContractBytecode { ... } and the Phoenix transaction's proof field now pretty prints as [ ... ].

@HDauven HDauven linked an issue Nov 20, 2024 that may be closed by this pull request
@moCello
Copy link
Member

moCello commented Nov 20, 2024

I understand that the contract byte-code and the transaction proof can pollute the output of the node-log.
Modifying the Debug trait for remedy isn't the right approach though as the Debug trait is meant for debugging purposes and printing the bytecode and proof can be beneficial in some circumstances.

Since this is an issue with the node log, it might make sense to also fix it in the node log and not on execution-core level where every change affects the entire network.

@d-sonuga d-sonuga force-pushed the remove-contract-deploy-noise-from-logs branch from 0f52b5a to 3e1c49b Compare November 20, 2024 13:58
@d-sonuga
Copy link
Collaborator Author

Message defined here and Payload defined here are the structs that get logged in node and node-data which may contain ContractBytecode.

I've changed the log messages to avoid outputting the entire structs.

@d-sonuga d-sonuga force-pushed the remove-contract-deploy-noise-from-logs branch from 3e1c49b to bdf9460 Compare November 20, 2024 14:07
Copy link
Member

@herr-seppia herr-seppia left a comment

Choose a reason for hiding this comment

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

I though that the originally issue were already solved by some @fed-franz PR

What are the logs that we're trying to truncate here?
I understood that they are related to the contract deployment, but I don't get which they are and how this PR would truncate them

Comment on lines +443 to +462
fn variant_name(&self) -> &'static str {
match self {
Payload::Block(_) => "Block",
Payload::Candidate(_) => "Candidate",
Payload::Empty => "Empty",
Payload::GetBlocks(_) => "GetBlocks",
Payload::GetMempool(_) => "GetMempool",
Payload::GetResource(_) => "GetResource",
Payload::Inv(_) => "Inv",
Payload::Quorum(_) => "Quorum",
Payload::Ratification(_) => "Ratification",
Payload::Transaction(_) => "Transaction",
Payload::Validation(_) => "Validation",
Payload::ValidationQuorum(_) => "ValidationQuorum",
Payload::ValidationResult(_) => "ValidationResult",
}
}
Copy link
Member

Choose a reason for hiding this comment

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

You can use msg.topic(), that implements Debug

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can't seem to find a topic function defined for node_data::message::Payload.

@d-sonuga
Copy link
Collaborator Author

d-sonuga commented Nov 20, 2024

@herr-seppia, the contract deployment holds a struct ContractBytecode which, in issue #2742, is a problem because of the raw contract bytes printed out which are irrelevant to node runners.

Message defined here and Payload defined here are structs which have chains of fields that result in a value possibly holding ContractBytecode.

These structs are printed out here and here.

This PR gets rid of this issue by avoiding the printing out of the entire struct which may contain ContractBytecode.

@herr-seppia
Copy link
Member

There is no trace of ContractDeployment in your changes.

The issue was created when the code was dumping the whole block (including transactions and optionally their contract deployment), but this has been changed by fed's pr

Hence my question.
Are you able to replicate the log that you want to truncate?

@d-sonuga
Copy link
Collaborator Author

@herr-seppia, yes I can replicate it.
Can you please send the link to the PR?

@herr-seppia
Copy link
Member

@herr-seppia, yes I can replicate it.

Perfect, can you post on the original issue how to?
Anyway, I'm against in shading the whole messages as you're proposing, but I would like to have a more punctual fix

@d-sonuga
Copy link
Collaborator Author

@herr-seppia, the replication is here.
And perhaps in addition to the new reduced warn/error logs in this PR, trace logs could be added to output more information?

@d-sonuga d-sonuga changed the title execution-core: truncate trace logs of contract deployments node, node-data: truncate trace logs of contract deployments Nov 20, 2024
@herr-seppia
Copy link
Member

@herr-seppia, the replication is here. And perhaps in addition to the new reduced warn/error logs in this PR, trace logs could be added to output more information?

The branch you proposed is actually throwing an error on purpose everytime a transaction is broadcasted.
My request was to use an official rusk node and try to reproduce the log pollution.

ps: Having the whole message dumped if an encoding error happens LGTM, because it's a bug that needs investigation

@d-sonuga
Copy link
Collaborator Author

Okay, since that's the case, I guess the issue should be considered resolved already?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Contract deployments pollute the log
3 participants