💬 Sjors commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1142032854)
The only other call site for `AddToBlockIndex()` is `LoadGenesisBlock()`, so I guess that's the only reason for the (generic looking) check of `hashPrevBlock`.

💬 MarcoFalke commented on issue "test: `wallet_importdescriptors.py --descriptors` failure under `--valgrind` ":
(https://github.com/bitcoin/bitcoin/issues/27282#issuecomment-1476131126)
What is the combined log?

💬 MarcoFalke commented on pull request "RPC: Fix fund transaction crash when at 0-value, 0-fee":
(https://github.com/bitcoin/bitcoin/pull/27271#issuecomment-1476131979)
Does this need backport?

💬 MarcoFalke commented on issue "test: `interface_bitcoin_cli.py --descriptors` failure under `--valgrind`":
(https://github.com/bitcoin/bitcoin/issues/27281#issuecomment-1476133389)
What is the combined log?

💬 fanquake commented on pull request "RPC: Fix fund transaction crash when at 0-value, 0-fee":
(https://github.com/bitcoin/bitcoin/pull/27271#issuecomment-1476134405)
> Does this need backport?

I think it's not-super-required but could be nice if we do.

💬 jamesob commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1142036850)
In most logging systems I've used elsewhere, (i) INFO is the default loglevel, and (ii) the level is configurable via flag/envvar. I guess what I'll do here is use `Warning` and leave a TODO.

💬 jamesob commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1142037129)
Good point! Thanks.

💬 Sjors commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1142037513)
Indeed this code is pretty ancient (and could use a comment): https://github.com/bitcoin/bitcoin/blame/f3ae51dcced8a16175426051ce888130cc2493af/src/main.cpp#L2056-L2063

💬 Sjors commented on issue "Memory leak in wallet_tests with BDB using address sanitizer":
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1476147317)
@fanquake that was 65840 bytes, this one is 320 bytes, so I assume it's a different issue?

I was going to add more details if anyone couldn't reproduce it, but here we go:

Found it while testing a PR, but reproduced on master @ 40e1c4d4024b8ad35f2511b2e10bf80c5531dfde.

```
Ubuntu clang version 15.0.6
Target: x86_64-pc-linux-gnu

Ubuntu 22.10 (GNU/Linux 6.0.6-060006-generic x86_64)
```

@MarcoFalke I'll see if I can get that too work.

💬 MarcoFalke commented on issue "Memory leak in wallet_tests with BDB using address sanitizer":
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1476150784)
This leak may be fixed in bdb5.3, but I haven't re-checked this.

💬 jamesob commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#issuecomment-1476162524)
Thanks for quick feedback, all. I *think* the latest update should make everyone happy.

💬 Sjors commented on issue "Memory leak in wallet_tests with BDB using address sanitizer":
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1476189108)
Narrowed it down to `BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)`.

💬 fanquake commented on issue "Memory leak in wallet_tests with BDB using address sanitizer":
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1476197268)
> I was going to add more details if anyone couldn't reproduce it, but here we go

In future, save us all wasting our time, and provide the (trivial to do so) details initially.

💬 virtu commented on pull request "mempool: Add mempool tracepoints":
(https://github.com/bitcoin/bitcoin/pull/26531#discussion_r1142083883)
In reply to the original comment: oversight on my part; will push a fix.

In reply to adding event timestamps for all tracepoints: in omitting the actual event timestamps as tracepoint parameters, I followed the precedent set by other tracepoints, where, as laid out by @0xB10C, event timestamps are generated inside tracing scripts.

💬 Sjors commented on issue "Memory leak in wallet_tests with BDB using address sanitizer":
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1476207042)
That test can be further narrowed down:

```
BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)
{
WalletContext context;
context.args = &m_args;
context.chain = m_node.chain.get();
auto wallet = TestLoadWallet(context);
// TestUnloadWallet(std::move(wallet));
// wallet = TestLoadWallet(context);
TestUnloadWallet(std::move(wallet));
}
```

Uncommenting the extra unload & load will trigger the sanitizer.

💬 dergoegge commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1142103379)
Wouldn't we also want to add the same logging to `ProcessHeadersMessage`? Thinking of BIP 130 block announcements.

💬 jamesob commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1142125339)
My thinking was that (i) legacy/low-bandwidth nodes are uninteresting for these logs in the sense that they are either in IBD or [...whatever prompts a node to be low-bandwidth-cmpctblock using]. And the log message in AcceptBlockHeader() obviously covers those cases too, and the lack of the cmpctblock message tells you that it's LB/legacy.

And it gets a little hairy because if you want to stick a log in ProcessHeadersMessage, you have to exclude the compactblock path because PHM() is called
...

💬 hebasto commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1476248917)
Updated ff6c0abeea00deffcaeaf3b02092110d6895b1c0 -> e580afee614b93236cb0e37457a5a7c28f4a0b73 ([pr26749.11](https://github.com/hebasto/bitcoin/commits/pr26749.11) -> [pr26749.12](https://github.com/hebasto/bitcoin/commits/pr26749.12), [diff](https://github.com/hebasto/bitcoin/compare/pr26749.11..pr26749.12)).

Addressed @martinus's performance nits:
- https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1141442527
- https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1141443409


...

💬 hebasto commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1142133225)
> The commit that fixes the use-after-free should come with a commit message to say that it fixed the bug

Thanks! [Updated](https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1476248917).

💬 hebasto commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1142133588)
Thanks! [Updated](https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1476248917).