💬 josibake commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1142002452)
This is because a lot of code was written before we introduced a style guide, or because it was missed during review. If something is not covered by the style guide, then we should try to be consistent with the surrounding code. If something _is_ covered by the style guide, we should be consistent with the style guide, even if that is not consistent with the surrounding code. This helps nudge the codebase towards the style guide over time.

💬 ajtowns commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1142002479)
How do you have a valid header without a known height? If you don't know all the ancestors, you can't tell if the `nBits` value is valid, which we check in `ContextualCheckBlockHeader` earlier in this function... (A missing prev block should result in `return Invalid(BLOCK_MISSING_PREV)` prior to that) The only case where we don't do all that is if we're looking at the genesis block header, but we know its height is 0... What am I missing here?

💬 josibake commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1142005645)
style nit: these should be lexicographically sorted

💬 jonatack commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1142007013)
- Good idea @Sjors, will do.

- "None" is internal-only (like Network::NET_MAX for iterating) and IIRC a commit in the severity -level logs parent PR ends up removing it

- "Info" is planned to be logged unconditionally as well, in addition to Warning and Error.

💬 ismaelsadeeq commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1142008651)
Thanks

💬 ismaelsadeeq commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1142009388)
Okay :+1:

⚠️ Sjors opened an issue: "Memory leak in wallet_tests with BDB using address sanitizer"
(https://github.com/bitcoin/bitcoin/issues/27283)
```
src/test/test_bitcoin --run_test=wallet_tests
```

Passed when configured without BDB, fails when configured with BDB:

```sh
./configure 'BDB_LIBS=-L/…' BDB_CFLAGS=-I/…/include CC=clang CXX=clang++ --enable-suppress-external-warnings --disable-asm --with-sanitizers=address --disable-fuzz-binary --without-gui
make -j33 -C src/test
src/test/test_bitcoin --run_test=wallet_tests

…

*** No errors detected

=================================================================
==63742
...

💬 Sjors commented on issue "Memory leak in wallet_tests with BDB using address sanitizer":
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1476101437)
cc @achow101

💬 fanquake commented on issue "Memory leak in wallet_tests with BDB using address sanitizer":
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1476110273)
Doesn't look like you've provided anything more actionable or useful than the last comment in #19034?

💬 josibake commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#issuecomment-1476110683)
ACK https://github.com/bitcoin/bitcoin/commit/9aac917c6e7fae48ceae5fe786c412a583e46d3e

Thanks for incorporating the feedback and addressing the style comments! Looks good.

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