💬 josibake commented on pull request "RPC: Fix fund transaction crash when at 0-value, 0-fee":
(https://github.com/bitcoin/bitcoin/pull/27271#discussion_r1141955223)
grammar nit:

```suggestion
// This can only happen if the feerate is 0, the requested destinations are value of 0 (e.g OP_RETURN),
// and there are no pre-selected inputs. This will result in a 0-input transaction, which is consensus-invalid

⚠️ fanquake opened an issue: "test: `wallet_importdescriptors.py --descriptors` failure under `--valgrind` "
(https://github.com/bitcoin/bitcoin/issues/27282)
Seen on a aarch64 Alpine box. Master @ https://github.com/bitcoin/bitcoin/commit/40e1c4d4024b8ad35f2511b2e10bf80c5531dfde. Binaries compiled with Clang 15.0.7. Valgrind `valgrind-3.21.0.GIT`.

We saw some issues with this test recently (#27229), but this looks like a different issue:

```bash
261/262 - wallet_importdescriptors.py --descriptors failed, Duration: 411 s

stdout:
2023-03-20T10:39:03.422000Z TestFramework (INFO): PRNG seed is: 4441145092714460381
2023-03-20T10:39:03.423000Z
...

💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1141966979)
> Hmm, that sounds like you are making it harder to remove cs_main.

I don't think this change complicates a refactor of the global locks. `zmqpublishnotifier` relies on `cs_main` being in global scope before and after this change. If the lock is moved into some narrower scope, this would have to be handled for the `zmqpublishnotifier` too - with our without this patch. If I read #27006 correctly, no change is required to make it work with the patch here.

> my brain enjoys the thought of ha
...

💬 willcl-ark commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1141968668)
Why do we need this special-cased 0 arg tracepoint? ISTM that the systemtap variadic supports 0 arg calls:

https://github.com/jav/systemtap/blob/2da355dd02a18bf4f67e2ceeb504b351b4bd5b83/includes/sys/sdt.h#L291-L297

💬 Sjors commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#issuecomment-1476049065)
Retracting ACK because I missed that `CalculateHeadersWork` doesn't _check_ the work, see https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1141510303

💬 ismaelsadeeq commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1141976587)
Yes, I agree with that, also I will use the `decode_segwit_address`.
removing the imports.

💬 Sjors commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1141977680)
Ah, TIL:

https://github.com/bitcoin/bitcoin/blob/40e1c4d4024b8ad35f2511b2e10bf80c5531dfde/src/logging.cpp#L127-L131

💬 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_r1141967974)
Since you changed the default for `apostrophe` in `FormatHDKeypath` to `false` this should be set to `true` here (as the below `WriteHDKeypaht` will already call it with `false`).

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