net: Log to net category for exceptions in ProcessMessages #17762

laanwj · 2019-12-17T15:09:27Z

Remove the forest of special exceptions based on string matching, and simply log a short message to the NET logging category when an exception happens during packet processing. It is not good to panick end users with verbose errors (let alone writing to stderr) when any peer can generate them.

Remove the forest of special exceptions, and simply log a short message to the NET logging category when an exception happens during packet processing. It is not good to panick end users with errors that any peer can generate (let alone writing to stderr).

practicalswift · 2019-12-17T15:31:50Z

Tested ACK 4d88c3d

maflcko · 2019-12-17T15:54:23Z

Concept ACK. I probably should have done this in #16021.

promag

ACK 4d88c3d.

promag · 2019-12-20T11:44:10Z

src/net_processing.cpp

    } catch (...) {
-        PrintExceptionContinue(nullptr, "ProcessMessages()");
+        LogPrint(BCLog::NET, "%s(%s, %u bytes): Unknown exception caught\n", __func__, SanitizeString(strCommand), nMessageSize);


nit, unexpected or unhandled instead or unknown?

It's really unknown. All known exceptions go to the other path. If it ends up here, some really weird code (maybe in a dependency?) threw an integer or bare string or such.

jnewbery

Concept ACK. One question about typeid() inline.

jnewbery · 2019-12-20T16:55:11Z

src/net_processing.cpp

-    catch (const std::exception& e) {
-        PrintExceptionContinue(&e, "ProcessMessages()");
+    } catch (const std::exception& e) {
+        LogPrint(BCLog::NET, "%s(%s, %u bytes): Exception '%s' (%s) caught\n", __func__, SanitizeString(strCommand), nMessageSize, e.what(), typeid(e).name());


What is typeid(e).name() for here? Here's what an example log looks like:

[msghand] ProcessMessages(tx, 85 bytes): Exception 'Superfluous witness record: iostream error' (NSt8ios_base7failureB5cxx11E) caught

Does that (NSt8ios_base7failureB5cxx11E) add any helpful information to the user?

According to https://en.cppreference.com/w/cpp/language/typeid, you need to include <typeinfo> to use typeid():

The header <typeinfo> must be included before using typeid (if the header is not included, every use of the keyword typeid makes the program ill-formed.)

And name():

Returns an implementation defined null-terminated character string containing the name of the type. No guarantees are given; in particular, the returned string can be identical for several types and change between invocations of the same program.

On Linux Mint 19.3 (GCC 7.4.0) with included <typeinfo> the output remains the same: something like ...(NSt8ios_base7failureB5cxx11E)....

Also we use the same approach here (line 544):

bitcoin/src/util/system.cpp

Lines 542 to 547 in 0cda557

if (pex)

return strprintf(

"EXCEPTION: %s \n%s \n%s in %s \n", typeid(*pex).name(), pex->what(), pszModule, pszThread);

else

return strprintf(

"UNKNOWN EXCEPTION \n%s in %s \n", pszModule, pszThread);

This was simply borrowed from the exception logging function that we've already used since Satoshi's days. I think the idea is that it shows the type in case the exception message is not informative, I'm ok with removing it, no opinion, but I don't think it hurts…

I'll add a commit that adds #include <typeinfo> to here and system.cpp. I think that's a good point, though no C++ compiler seems to have stumbled over it.

maflcko · 2019-12-20T17:04:46Z

ACK 4d88c3d 👃

Tested both cases:

Normal exception:

2019-12-20T16:50:10.551972Z [msghand] received: tx (85 bytes) peer=2
2019-12-20T16:50:10.552100Z [msghand] ProcessMessages(tx, 85 bytes): Exception 'Superfluous witness record: iostream error' (NSt8ios_base7failureB5cxx11E) caught
2019-12-20T16:50:10.552120Z [msghand] ProcessMessages(tx, 85 bytes) FAILED peer=2

Something else:

2019-12-20T17:03:18.898307Z [msghand] received: tx (85 bytes) peer=2
2019-12-20T17:03:18.898406Z [msghand] ProcessMessages(tx, 85 bytes): Unknown exception caught
2019-12-20T17:03:18.898421Z [msghand] ProcessMessages(tx, 85 bytes) FAILED peer=2

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 4d88c3dcb61e7c075ed3dd442044e0eff4e3c8de 👃

Tested both cases:

* Normal exception:

 - 2019-12-20T16:50:10.551972Z [msghand] received: tx (85 bytes) peer=2
 - 2019-12-20T16:50:10.552100Z [msghand] ProcessMessages(tx, 85 bytes): Exception 'Superfluous witness record: iostream error' (NSt8ios_base7failureB5cxx11E) caught
 - 2019-12-20T16:50:10.552120Z [msghand] ProcessMessages(tx, 85 bytes) FAILED peer=2

* Something else:

 - 2019-12-20T17:03:18.898307Z [msghand] received: tx (85 bytes) peer=2
 - 2019-12-20T17:03:18.898406Z [msghand] ProcessMessages(tx, 85 bytes): Unknown exception caught
 - 2019-12-20T17:03:18.898421Z [msghand] ProcessMessages(tx, 85 bytes) FAILED peer=2
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjeMwwArad7Z2G99LooWjtt7/fr7X2kNavsjZh1YvI8AytS/9uZkURdVFg96geY
Q3jJqisfbeNTwJm0zsNbSWChN5OlWrZ8naGMDfyALM0oQXzj5rgCZM5WIZta4Me9
XDFq4upRfvg5QhmFI1feGX4mIYqdf2YlxYjkLz293rvTRTnQsEJ655tk1uWrm1jK
SnGKtv29IKNO0yEb4WOE4pB7muzlf3mXVO9K+21HglK5qY3Opy7hkO8NsJSpilvz
Gr4Y8yJFhJMWL8gG3fZb/YaViDzcNanbjMNXTUHIwMQZ6sQhOpDvXKYNvyNJG8Di
1xleSq1lEHp/nAdPor29fMCLVaSi7EY7A32LetW7SE30I6zbqlbH3XW4rg/mgiHr
bzoz/yJT6vrxRm8BOLYu9PjQLhKOVeDL0iGmxQaHxUdnpDdlQUYgjTeCcGUFMptB
vwD7xTEfgFdeBQuDjwFccpDDgUdUO8FBtFBHxxOEa1VVHtEvteRRvMzXCnJFaR0K
NONhQti2
=lSTE
-----END PGP SIGNATURE-----

Timestamp of file with hash df6e3a596aced982ab075caecb0b392dea1ae5573fef22c74d4cdd198ffc188c -

hebasto · 2019-12-20T19:22:21Z

Concept ACK.

The use of `typeid()` for logging exception types requires this include according to https://en.cppreference.com/w/cpp/language/typeid.

maflcko · 2020-01-02T20:15:12Z

re-ACK 4bdd68f (only change is adding includes) 🕕

Show signature and timestamp

Signature:

Timestamp of file with hash f5a70ac73ac8d01fd424479a9ace0934a6de8392add537503e92e054731e2b32 -

promag · 2020-01-02T20:18:52Z

ACK 4bdd68f, could squash.

4bdd68f Add missing typeinfo includes (Wladimir J. van der Laan) 4d88c3d net: Log to net category for exceptions in ProcessMessages (Wladimir J. van der Laan) Pull request description: Remove the forest of special exceptions based on string matching, and simply log a short message to the NET logging category when an exception happens during packet processing. It is not good to panick end users with verbose errors (let alone writing to stderr) when any peer can generate them. ACKs for top commit: MarcoFalke: re-ACK 4bdd68f (only change is adding includes) 🕕 promag: ACK 4bdd68f, could squash. Tree-SHA512: a005591a3202b005c75e01dfa54249db3992e2f9eefa8b3d9d435acf66130417716ed926ce4e045179cf43788f1abc7362d999750681a9c80b318373d611c366

…ssMessages 4bdd68f Add missing typeinfo includes (Wladimir J. van der Laan) 4d88c3d net: Log to net category for exceptions in ProcessMessages (Wladimir J. van der Laan) Pull request description: Remove the forest of special exceptions based on string matching, and simply log a short message to the NET logging category when an exception happens during packet processing. It is not good to panick end users with verbose errors (let alone writing to stderr) when any peer can generate them. ACKs for top commit: MarcoFalke: re-ACK 4bdd68f (only change is adding includes) 🕕 promag: ACK 4bdd68f, could squash. Tree-SHA512: a005591a3202b005c75e01dfa54249db3992e2f9eefa8b3d9d435acf66130417716ed926ce4e045179cf43788f1abc7362d999750681a9c80b318373d611c366

Remove the forest of special exceptions, and simply log a short message to the NET logging category when an exception happens during packet processing. It is not good to panick end users with errors that any peer can generate (let alone writing to stderr). Github-Pull: bitcoin#17762 Rebased-From: 4d88c3d

The use of `typeid()` for logging exception types requires this include according to https://en.cppreference.com/w/cpp/language/typeid. Github-Pull: bitcoin#17762 Rebased-From: 4bdd68f

fanquake · 2020-01-05T23:59:12Z

Being backported in 17858.

99b5407 scripts: fix check-symbols & check-security argument passing (fanquake) 4330a1e Update msvc build for Visual Studio 2019 v16.4 (Aaron Clauson) b0f9b8e Moves vcpkg list to a text file and updates the appveyor job and readme to use it. (Aaron Clauson) cd7b3b2 Updated appveyor config: - Update build image from Visual Studio 2017 to Visual Studio 2019. - Updated Qt static library from Qt5.9.7 to Qt5.9.8. - Added commands to update vcpkg port files (this does not update already installed packages). - Updated vcpkg package list as per #17309. - Removed commands setting common project file options. Now done via common.init.vcxproj include. - Changed msbuild verbosity from normal to quiet. Normal rights a LOT of logs and impacts appveyor job duration. Updated msvc project configs: - Updated platform toolset from v141 to v142. - Updated Qt static library from Qt5.9.7 to Qt5.9.8. - Added ignore for linker warning building bitcoin-qt program. - Added missing util/str.cpp class file to test_bitcoin project file. (Aaron Clauson) 112144d Add missing typeinfo includes (Wladimir J. van der Laan) 1a6a534 net: Log to net category for exceptions in ProcessMessages (Wladimir J. van der Laan) c0dc728 test: fix bitcoind already running warnings on macOS (fanquake) 5276b0e util: Add missing headers to util/fees.cpp (Hennadii Stepanov) 4d7875c rpc: require second argument only for scantxoutset start action (Andrew Chow) bda2f5b cli: fix Fatal LevelDB error when specifying -blockfilterindex=basic twice (Harris) d14ab7c gui: disable File->CreateWallet during startup (fanquake) b9f1bc0 wallet: unbreak with boost 1.72 (Jan Beich) Pull request description: Backports the following PRs to the 0.19 branch: * #17654 - Unbreak build with Boost 1.72.0 * #17695 - gui: disable File->CreateWallet during startup * #17687 - cli: fix Fatal LevelDB error when specifying -blockfilterindex=basic twice * #17728 - rpc: require second argument only for scantxoutset start action * #17450 - util: Add missing headers to util/fees.cpp * #17488 - test: fix "bitcoind already running" warnings on macOS * #17762 - Log to net category for exceptions in ProcessMessages * #17364 - Updates to appveyor config for VS2019 and Qt5.9.8 + msvc project fixes * #17416 - Appveyor improvement - text file for vcpkg package list * #17736 - Update msvc build for Visual Studio 2019 v16.4 * #17857 - scripts: fix symbol-check & security-check argument passing Fixes #17856. ACKs for top commit: sipsorcery: ACK (tested: Windows 10 & msvc build) 99b5407. Tree-SHA512: 91313de56fb0825e70a4be30ba0bf561b8c26d7dcf60549185df4f5e3524099398c828bb46faae807b631634d1afd5a1d397fb41e61ecfa0d746e4bf10b923cb

Remove the forest of special exceptions, and simply log a short message to the NET logging category when an exception happens during packet processing. It is not good to panick end users with errors that any peer can generate (let alone writing to stderr). Github-Pull: bitcoin#17762 Rebased-From: 4d88c3d

practicalswift · 2020-01-22T17:32:25Z

Reviewers of this PR might want to review the somewhat related PR #17828 ("net: Use log categories when logging events that P2P peers can trigger arbitrarily").

…ssMessages c89611e net: Log to net category for exceptions in ProcessMessages (Wladimir J. van der Laan) Pull request description: Backport of #17762, currently only backported to 0.19. This seems like something we should opportunistically plug in case wiseguys decide it's a vector to exploit to try and fill people's disks. ACKs for top commit: practicalswift: ACK c89611e MarcoFalke: ACK c89611e, checked that this is a cherry-pick from 0.19 💐 Tree-SHA512: e48daf64a14d98a78cadd0774a597e5833a1ff19f05527dfc42f3cc38532c1c3bd1acd925c8e0c484e01fbc8c604ee2bcfc0cec0333e9af570b103a6241b657d

fanquake · 2020-01-23T02:08:36Z

This was backported to the 0.18 branch in #17974.

[0.19] Backports bitcoin#17858 Unbreak build with Boost 1.72.0 bitcoin#17654 cli: fix Fatal LevelDB error when specifying -blockfilterindex=basic twice bitcoin#17687 rpc: require second argument only for scantxoutset start action bitcoin#17728 wallet: Fix origfee return for bumpfee with feerate arg bitcoin#17643 test: fix "bitcoind already running" warnings on macOS bitcoin#17488 net: Log to net category for exceptions in ProcessMessages bitcoin#17762 Updates to appveyor config for VS2019 and Qt5.9.8 + msvc project fixes bitcoin#17364 Appveyor improvement - text file for vcpkg package list bitcoin#17416 Update msvc build for Visual Studio 2019 v16.4 bitcoin#17736 scripts: fix symbol-check & security-check argument passing bitcoin#17857 qt: Periodic translations update for 0.19 branch IsUsedDestination should count any known single-key address bitcoin#17621 init: Stop indexes on shutdown after ChainStateFlushed callback. bitcoin#17897 qt: Translations update pre-rc1 wallet: Reset reused transactions cache bitcoin#17843 Squashed 'src/univalue/' changes from 7890db9..98261b1 0.19: Update univalue subtree bitcoin#18100 qt: Pre-rc2 translations update

[0.19] Backports bitcoin#17858 Unbreak build with Boost 1.72.0 bitcoin#17654 cli: fix Fatal LevelDB error when specifying -blockfilterindex=basic twice bitcoin#17687 rpc: require second argument only for scantxoutset start action bitcoin#17728 wallet: Fix origfee return for bumpfee with feerate arg bitcoin#17643 test: fix "bitcoind already running" warnings on macOS bitcoin#17488 net: Log to net category for exceptions in ProcessMessages bitcoin#17762 Updates to appveyor config for VS2019 and Qt5.9.8 + msvc project fixes bitcoin#17364 Appveyor improvement - text file for vcpkg package list bitcoin#17416 Update msvc build for Visual Studio 2019 v16.4 bitcoin#17736 scripts: fix symbol-check & security-check argument passing bitcoin#17857 qt: Periodic translations update for 0.19 branch IsUsedDestination should count any known single-key address bitcoin#17621 init: Stop indexes on shutdown after ChainStateFlushed callback. bitcoin#17897 qt: Translations update pre-rc1 wallet: Reset reused transactions cache bitcoin#17843 Squashed 'src/univalue/' changes from 7890db9..98261b1 0.19: Update univalue subtree bitcoin#18100 qt: Pre-rc2 translations update [0.19] Further 0.19 backports bitcoin#18218 build: don't embed a build-id when building libdmg-hfsplus bitcoin#18004

…ssMessages 4bdd68f Add missing typeinfo includes (Wladimir J. van der Laan) 4d88c3d net: Log to net category for exceptions in ProcessMessages (Wladimir J. van der Laan) Pull request description: Remove the forest of special exceptions based on string matching, and simply log a short message to the NET logging category when an exception happens during packet processing. It is not good to panick end users with verbose errors (let alone writing to stderr) when any peer can generate them. ACKs for top commit: MarcoFalke: re-ACK 4bdd68f (only change is adding includes) 🕕 promag: ACK 4bdd68f, could squash. Tree-SHA512: a005591a3202b005c75e01dfa54249db3992e2f9eefa8b3d9d435acf66130417716ed926ce4e045179cf43788f1abc7362d999750681a9c80b318373d611c366

Summary: > Remove the forest of special exceptions, and simply log a short > message to the NET logging category when an exception happens during > packet processing. It is not good to panick end users with errors > that any peer can generate (let alone writing to stderr). > > Add missing typeinfo includes > The use of `typeid()` for logging exception types requires this include > according to https://en.cppreference.com/w/cpp/language/typeid. This is a backport of Core [[bitcoin/bitcoin#17762 | PR17762]] Test Plan: `ninja && ninja check` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8390

99b5407 scripts: fix check-symbols & check-security argument passing (fanquake) 4330a1e Update msvc build for Visual Studio 2019 v16.4 (Aaron Clauson) b0f9b8e Moves vcpkg list to a text file and updates the appveyor job and readme to use it. (Aaron Clauson) cd7b3b2 Updated appveyor config: - Update build image from Visual Studio 2017 to Visual Studio 2019. - Updated Qt static library from Qt5.9.7 to Qt5.9.8. - Added commands to update vcpkg port files (this does not update already installed packages). - Updated vcpkg package list as per bitcoin#17309. - Removed commands setting common project file options. Now done via common.init.vcxproj include. - Changed msbuild verbosity from normal to quiet. Normal rights a LOT of logs and impacts appveyor job duration. Updated msvc project configs: - Updated platform toolset from v141 to v142. - Updated Qt static library from Qt5.9.7 to Qt5.9.8. - Added ignore for linker warning building bitcoin-qt program. - Added missing util/str.cpp class file to test_bitcoin project file. (Aaron Clauson) 112144d Add missing typeinfo includes (Wladimir J. van der Laan) 1a6a534 net: Log to net category for exceptions in ProcessMessages (Wladimir J. van der Laan) c0dc728 test: fix bitcoind already running warnings on macOS (fanquake) 5276b0e util: Add missing headers to util/fees.cpp (Hennadii Stepanov) 4d7875c rpc: require second argument only for scantxoutset start action (Andrew Chow) bda2f5b cli: fix Fatal LevelDB error when specifying -blockfilterindex=basic twice (Harris) d14ab7c gui: disable File->CreateWallet during startup (fanquake) b9f1bc0 wallet: unbreak with boost 1.72 (Jan Beich) Pull request description: Backports the following PRs to the 0.19 branch: * bitcoin#17654 - Unbreak build with Boost 1.72.0 * bitcoin#17695 - gui: disable File->CreateWallet during startup * bitcoin#17687 - cli: fix Fatal LevelDB error when specifying -blockfilterindex=basic twice * bitcoin#17728 - rpc: require second argument only for scantxoutset start action * bitcoin#17450 - util: Add missing headers to util/fees.cpp * bitcoin#17488 - test: fix "bitcoind already running" warnings on macOS * bitcoin#17762 - Log to net category for exceptions in ProcessMessages * bitcoin#17364 - Updates to appveyor config for VS2019 and Qt5.9.8 + msvc project fixes * bitcoin#17416 - Appveyor improvement - text file for vcpkg package list * bitcoin#17736 - Update msvc build for Visual Studio 2019 v16.4 * bitcoin#17857 - scripts: fix symbol-check & security-check argument passing Fixes bitcoin#17856. ACKs for top commit: sipsorcery: ACK (tested: Windows 10 & msvc build) 99b5407. Tree-SHA512: 91313de56fb0825e70a4be30ba0bf561b8c26d7dcf60549185df4f5e3524099398c828bb46faae807b631634d1afd5a1d397fb41e61ecfa0d746e4bf10b923cb

laanwj added the P2P label Dec 17, 2019

promag reviewed Dec 20, 2019

View reviewed changes

jnewbery reviewed Dec 20, 2019

View reviewed changes

practicalswift mentioned this pull request Dec 29, 2019

net: Use log categories when logging events that P2P peers can trigger arbitrarily #17828

Closed

Add missing typeinfo includes

4bdd68f

The use of `typeid()` for logging exception types requires this include according to https://en.cppreference.com/w/cpp/language/typeid.

laanwj merged commit 4bdd68f into bitcoin:master Jan 2, 2020

laanwj added the Needs backport (0.19) label Jan 4, 2020

fanquake mentioned this pull request Jan 4, 2020

[0.19] Backports #17858

Merged

fanquake removed the Needs backport (0.19) label Jan 5, 2020

instagibbs mentioned this pull request Jan 21, 2020

[0.18] net: Log to net category for exceptions in ProcessMessages #17974

Merged

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

	if (pex)
	return strprintf(
	"EXCEPTION: %s \n%s \n%s in %s \n", typeid(*pex).name(), pex->what(), pszModule, pszThread);
	else
	return strprintf(
	"UNKNOWN EXCEPTION \n%s in %s \n", pszModule, pszThread);

net: Log to net category for exceptions in ProcessMessages #17762

net: Log to net category for exceptions in ProcessMessages #17762

Uh oh!

Conversation

laanwj commented Dec 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Dec 17, 2019

Uh oh!

maflcko commented Dec 17, 2019

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Dec 20, 2019

Choose a reason for hiding this comment

Uh oh!

laanwj Jan 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery Dec 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Dec 20, 2019

Choose a reason for hiding this comment

Uh oh!

hebasto Dec 20, 2019

Choose a reason for hiding this comment

Uh oh!

laanwj Jan 2, 2020

Choose a reason for hiding this comment

Uh oh!

laanwj Jan 2, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko commented Dec 20, 2019

Uh oh!

hebasto commented Dec 20, 2019

Uh oh!

maflcko commented Jan 2, 2020

Uh oh!

promag commented Jan 2, 2020

Uh oh!

fanquake commented Jan 5, 2020

Uh oh!

practicalswift commented Jan 22, 2020

Uh oh!

fanquake commented Jan 23, 2020

Uh oh!

Uh oh!

laanwj commented Dec 17, 2019 •

edited

Loading

laanwj Jan 2, 2020 •

edited

Loading

jnewbery Dec 20, 2019 •

edited

Loading