💬 ismaelsadeeq commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1141979331)
Thanks for pointing that out :100:

💬 darosior commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#issuecomment-1476060387)
Concept ACK.

💬 ismaelsadeeq commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1141980347)
still, for consistency, I think it's good.

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

💬 Sjors commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1141986299)
@jonatack it might be useful to expand the `debug.log` section in developer notes with some guidance. E.g. it seems good practice to always use `LogPrintfCategory`, always pick a category and then use `>= BCLog::Level::Warning` for things you always want in the logs even when the user did not pick the specific `-debug` setting.

@ajtowns I think `BCLog::Level:None` is off-limits, from the description of f1379aeca9d3a8c4d3528de4d0af8298cb42fee4: "replace the unused BCLog::Level:None string "non
...

💬 fanquake commented on pull request "log: expand BCLog::LogFlags (categories) to 64 bits":
(https://github.com/bitcoin/bitcoin/pull/26619#issuecomment-1476076226)
> but the current PR may be preferable after all

Looks like the concerns expressed in #26697, which were reason for reopening here, are no-longer valid? Going to reclose this.

✅ fanquake closed a pull request: "log: expand BCLog::LogFlags (categories) to 64 bits"
(https://github.com/bitcoin/bitcoin/pull/26619)

💬 Sjors commented on pull request "Switch hardened derivation marker to h (in normalized descriptors and new wallets)":
(https://github.com/bitcoin/bitcoin/pull/26076#discussion_r1141994246)
I suppose it would be better if legacy wallet RPC calls don't change behavior. I'll look into this (and the other comments above).

💬 ismaelsadeeq commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1141997488)
functions in the codebase have one blank line in between.

💬 josibake commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1141998972)
The goal is to have the code base consistent with our style guide, not consistent with itself. This means any new code should adhere to the style guide and, if possible, old code should be formatted consistent with the style guide if it's already being touched as part of a PR.

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

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