💬 Sjors commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1142021516)
`AddToBlockIndex` checks if `hashPrevBlock` is already in the index, and if not it won't set `pprev` and `nHeight` on the `CBlockIndex` entry it returns. But I didn't check if that condition can ever be reached from the call in `AcceptBlockHeader`.

💬 fanquake commented on issue "Memory leak in wallet_tests with BDB using address sanitizer":
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1476116230)
Also if you are going to open issues like this, can you provide more information. i.e you don't even tell us what version of the code this happens with. I guess we are meant to assume it's the latest commit in master? What compiler versions did you use? What system are you running on?

💬 MarcoFalke commented on issue "Memory leak in wallet_tests with BDB using address sanitizer":
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1476120851)
>There's currently no suppression file for the address sanitizer

See https://github.com/bitcoin/bitcoin/blob/master/test/sanitizer_suppressions/lsan

💬 fanquake commented on pull request "test: Use consistent assert functions":
(https://github.com/bitcoin/bitcoin/pull/26356#issuecomment-1476122672)
Closing for now. Looks like the user has also disspeared off GitHub.

✅ fanquake closed a pull request: "test: Use consistent assert functions"
(https://github.com/bitcoin/bitcoin/pull/26356)

💬 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.