💬 darosior commented on pull request "Switch hardened derivation marker to h (in normalized descriptors and new wallets)":
(https://github.com/bitcoin/bitcoin/pull/26076#discussion_r1141889186)
nit: this reads like this change made it possible to use `h` as a marker when passing a descriptor as an RPC command argument. But it was already possible before. This PR only changes the RPC response. (Although making it indirectly simpler for when you copy paste from an RPC response directly to an RPC command's argument).

💬 darosior commented on pull request "Switch hardened derivation marker to h (in normalized descriptors and new wallets)":
(https://github.com/bitcoin/bitcoin/pull/26076#discussion_r1141976893)
This is an API break? (Albeit a small one)

💬 darosior commented on pull request "Switch hardened derivation marker to h (in normalized descriptors and new wallets)":
(https://github.com/bitcoin/bitcoin/pull/26076#discussion_r1141973833)
nit: `false` is already the default for `FormatHDKeypath`.

💬 ismaelsadeeq commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1141978833)
I think for consistency in the code base it's okay.
There is only one blank line between functions in address.py

💬 fanquake commented on pull request "refactor: remove unused param from legacy pubkey interface":
(https://github.com/bitcoin/bitcoin/pull/27274#issuecomment-1476059294)
```bash
if (!ReserveKeyFromKeyPool(nIndex, keypool, /*internal=*/ false) && !m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
^
./wallet/scriptpubkeyman.h:353:73: note: 'fRequestedInternal' declared here
bool ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool fRequestedInternal);
^
clang-tidy-14 --use-color -p=/tmp/cirrus-ci-build
...

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