log: Avoid breaking single log lines over multiple lines in the log file #23104

pull MarcoFalke wants to merge 4 commits into bitcoin:master from MarcoFalke:2109-logCont changing 4 files +16 −13

MarcoFalke commented at 11:49 AM on September 27, 2021: member

Follow up to commit d8b4b3077fd20c90b635eff1dd240bdad9725027

log: Avoid broken SELECTCOINS log

Before this patch, the log might be corrupted by other threads logging
at the same time. For example, another RPC thread:

[httpworker.1] [default wallet] keypool reserve 1296
[httpworker.1] SelectCoins() best subset: Received a POST request for / from 127.0.0.1:53732
[httpworker.3] ThreadRPCServer method=getnetworkinfo user=__cookie__
[httpworker.1] 0.78125 0.1953125 0.02441406 0.00610351 0.00305175 0.00152587 total 1.01025417

faffaa85cd

laanwj commented at 12:03 PM on September 27, 2021: member

Code review ACK fa00654e3783d5c2d93ddeb0b06f26fe2ef885d8 re-ACK 2222c04e1b9960030cb590c789a0d2375add4544
laanwj added the label Utils/log/libs on Sep 27, 2021
in src/wallet/coinselection.cpp:282 in fa2c4839db outdated
```
 278 | @@ -279,13 +279,13 @@ bool KnapsackSolver(const CAmount& nTargetValue, std::vector<OutputGroup>& group
 279 |          }
 280 |  
 281 |          if (LogAcceptCategory(BCLog::SELECTCOINS)) {
 282 | -            LogPrint(BCLog::SELECTCOINS, "SelectCoins() best subset: "); /* Continued */
 283 | +            std::string log_message{"SelectCoins() best subset: "};
```
laanwj commented at 12:29 PM on September 27, 2021:

Not introduced in this change, but this function name is wrong. Should be KnapsackSolver()? Or simply __func__.

MarcoFalke commented at 12:41 PM on September 27, 2021:

I'll let @achow101 decide

achow101 commented at 4:12 PM on September 27, 2021:

__func__

MarcoFalke commented at 6:06 AM on September 28, 2021:

On a second thought, it is already possible to log the function name with -logsourcelocations, so maybe just remove it?

MarcoFalke commented at 6:24 AM on September 28, 2021:

Resolving discussion for now. Let me know if anything needs to be changed after the latest commit fad8481
log: Avoid broken DEBUG_LOCKORDER log faeae2980f
test: Fix typos in tests fa6c1e850f

log: Adjust coin selection log string

Replace the outdated function name with words from the English language.
Logging the function name can be toggled with -logsourcelocations.

2222c04e1b

in src/sync.cpp:100 in fad8481053 outdated
```
  96 | @@ -97,27 +97,29 @@ static void potential_deadlock_detected(const LockPair& mismatch, const LockStac
  97 |      LogPrintf("POTENTIAL DEADLOCK DETECTED\n");
  98 |      LogPrintf("Previous lock order was:\n");
  99 |      for (const LockStackItem& i : s1) {
 100 | +        std::string_view prefix{};
```
practicalswift commented at 3:18 PM on September 29, 2021:

Is there any compelling reason to use the "potentially dangerous" std::string_view over std::string here? (Not claiming that this the use here is dangerous as currently written.)

Lifetime issues are scary and personally I find it much easier to rule out lifetime issues when reviewing code that uses std::string compared to code that uses std::string_view :)

Somewhat related: std::string_view encourages use-after-free

MarcoFalke commented at 3:33 PM on September 29, 2021:

string_view is no different than span, which is also in our code base. I personally prefer string_view over c-strings (raw pointers).

Luckily, if review doesn't catch use-after-free, we have compilers that check the lifetimebound attribute (clang), as well as runtime sanitizers.

MarcoFalke commented at 4:06 PM on September 29, 2021:

Also, this is only used in tests and the program will std::abort anyway right after, so I claim it wouldn't matter if a use-after-free made it in (despite all QA).

MarcoFalke commented at 4:50 PM on September 29, 2021:

Force pushed std::string to avoid the discussion :sweat_smile:
MarcoFalke force-pushed on Sep 29, 2021
practicalswift commented at 7:54 AM on September 30, 2021: contributor

cr ACK 2222c04e1b9960030cb590c789a0d2375add4544

Thanks for removing the use of std::string_view (context): FWIW that changed my cost-benefit analysis of this PR to transition from benefit < cost to benefit > cost :)
laanwj merged this on Sep 30, 2021
laanwj closed this on Sep 30, 2021
sidhujag referenced this in commit 544b0c66c4 on Sep 30, 2021
MarcoFalke deleted the branch on Oct 1, 2021
DrahtBot locked this on Oct 30, 2022

Contributors

Labels