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

Log "nn.nnn coins spent using strategy ..." on log level Debug #326

Merged
merged 2 commits into from
Nov 19, 2017

Conversation

sesam
Copy link
Contributor

@sesam sesam commented Nov 19, 2017

Issue #288 says that, for now, we'd want to log total amount sent.

Should I change any of

  • log level = Debug
  • include destination? Currently not listed for log file brevity and privacy

@ignopeverell
Copy link
Contributor

I think the idea in #288 was to make this accessible to wallet info, which means a separate data file, similar to wallet.dat that captures transaction-level information. It could say something like "sent X coins on Y date to Z addres, outputs A, B, C where consumed and D change was created".

@sesam
Copy link
Contributor Author

sesam commented Nov 19, 2017

tl;dr roadworks. Will try adding a new and separate outputs_spent_logger or maybe better transactions_logger to get going.

Oh I see. To log (at minimum) total amount spent via util::logger, using slog, is probably cleanest done by instantiating an additional logger, completely separate from the util::logger helper.

There less clean option would be to bolt in one extra drain that is craftily filtered. But that means also terminal and file output filters must be filtered, so the filtering latency will be applied to all logging. Filtering by referencing the wanted logger instance from the beginning is of course more efficient, and a lot less messy.

slog quick review

  • + nice
  • + nifty drain abstraction (but "cost free", really?)
  • + supports filtering by module and line number (via environment variables, so that feature is compiled in by default) as well as runtime log filtering (google: Drain for RuntimeLevelFilter)
  • + key-values can be attached to each log line, and be saved machine-readable, which could be a nice way to get good data to use for batch- and CI verification of executed transactions vs final wallet.dat contents
  • - likely overkill
  • - any filtering latency/penalties are paid by all log! / debug! / ... calls
  • - attack surface
  • - release builds contain all code, whether statically filtered out (release_max_level_...) or not
  • + (?) release builds contain all code, just like the core bitcoind builds also prefer to do
  • - no log rotation support, nor compression. slog docs point to this being a suitable tag:easy tag:helpwanted future task)

I'm imagining we can find a simpler logging solution with smaller potential attack surface later on, or roll our own, though I'm not suggesting to refactor and disturb everyone's stashes and branches right now.

Add code comment about LOGGING_CONFIG injection
@sesam sesam force-pushed the log-coin-spend-amount branch from bf4358a to 1a25097 Compare November 19, 2017 14:06
@ignopeverell
Copy link
Contributor

Okay sorry, I should have been more explicit. To be easily usable by the wallet it should be some JSON data. With that the wallet could display some info but also rollback some failed transactions for example. So I didn't mean "it could say" literally, I was just trying to describe the type of information that would fit in there.

@ignopeverell
Copy link
Contributor

Still going to merge this as having some minimal logging is better than nothing until we can have a more formal solution. Maybe extending the format of wallet.dat would make the most sense.

@ignopeverell ignopeverell merged commit 08a277f into mimblewimble:master Nov 19, 2017
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.

2 participants