Skip to content

chainparams: do not log signet startup messages for other chains#20048

Merged
maflcko merged 1 commit intobitcoin:masterfrom
jonatack:signet-startup-logging-bugfix
Oct 1, 2020
Merged

chainparams: do not log signet startup messages for other chains#20048
maflcko merged 1 commit intobitcoin:masterfrom
jonatack:signet-startup-logging-bugfix

Conversation

@jonatack
Copy link
Member

@jonatack jonatack commented Sep 30, 2020 •
edited
Loading

The following signet startup messages are printed to the debug log immediately on node startup for all chains. This behavior occurs on master as a side effect after the merge of #20014. This PR removes the first message and moves the signet derived magic logging to init.cpp.

$ ./src/bitcoind
2020-09-30T14:25:15Z Using default signet network
2020-09-30T14:25:15Z Signet derived magic (message start): 0a03cf40
2020-09-30T14:25:15Z Bitcoin Core version v0.20.99.0 ...

@maflcko
Copy link
Member

maflcko commented Sep 30, 2020

Seems odd for a constructor to log anyway. I'd say to remove them or move them elsewhere

@jonatack
Copy link
Member Author

jonatack commented Sep 30, 2020

Makes sense, but I didn't move them because they are related to the values being set, and because the first message interrelates with the other -signetchallenge messages and value settings. Pinging @kallewoof for thoughts.

@practicalswift
Copy link
Contributor

practicalswift commented Sep 30, 2020

Concept ACK

Agree with MarcoFalke regarding removing them from the ctor.

@hebasto
Copy link
Member

hebasto commented Sep 30, 2020

Concept ACK.

kallewoof
kallewoof reviewed Oct 1, 2020
View reviewed changes
Copy link
Contributor

@kallewoof kallewoof left a comment

Choose a reason for hiding this comment

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

Concept and utACK

@kallewoof
Copy link
Contributor

kallewoof commented Oct 1, 2020

Removing is fine, but they're useful esp for custom signets since you need the magic for external software.

@maflcko
Copy link
Member

maflcko commented Oct 1, 2020

The magic is logged unconditionally in the constructor. If needed, it can be logged unconditionally in the startup-sequence, e.g. init.cpp

@jonatack
signet: do not log signet startup messages for other chains
6fccad7 
and move signet network magic logging from chainparams.cpp to init.cpp
@jonatack jonatack force-pushed the signet-startup-logging-bugfix branch from 8fdcadb to 6fccad7 Compare October 1, 2020 09:27
@jonatack
Copy link
Member Author

jonatack commented Oct 1, 2020

Updated proposal: removed the first message and moved the signet network magic message to init.cpp.

kallewoof
kallewoof approved these changes Oct 1, 2020
View reviewed changes
Copy link
Contributor

@kallewoof kallewoof left a comment

Choose a reason for hiding this comment

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

utACK 6fccad7

hebasto
hebasto approved these changes Oct 1, 2020
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK 6fccad7

Maybe also add testing for not appearing signet messages in the log if not on signet?

@maflcko
Copy link
Member

maflcko commented Oct 1, 2020

ACK 6fccad7

@maflcko maflcko merged commit 9fc2f01 into bitcoin:master Oct 1, 2020
@jonatack jonatack deleted the signet-startup-logging-bugfix branch October 1, 2020 11:34
@jonatack
Copy link
Member Author

jonatack commented Oct 1, 2020

Maybe also add testing for not appearing signet messages in the log if not on signet?

TBH was tempted but thought even the one added test might be overkill.

@maflcko
Copy link
Member

maflcko commented Oct 1, 2020

Maybe also add testing for not appearing signet messages in the log if not on signet?

I just tested this manually, and it seems unlikely and low-risk to regress

@jonatack jonatack mentioned this pull request Oct 1, 2020
Closed
@ryanofsky ryanofsky mentioned this pull request Apr 12, 2021
Draft
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@hebasto hebasto hebasto approved these changes

+1 more reviewer

@kallewoof kallewoof kallewoof approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Validation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jonatack @maflcko @practicalswift @hebasto @kallewoof @DrahtBot