💬 fanquake commented on issue "feature_config_args.py failure under `--valgrind`":
(https://github.com/bitcoin/bitcoin/issues/27259#issuecomment-1476035165)
Ok this is timeout related. Going to close for now.

✅ fanquake closed an issue: "feature_config_args.py failure under `--valgrind`"
(https://github.com/bitcoin/bitcoin/issues/27259)

👍 josibake approved a pull request: "RPC: Fix fund transaction crash when at 0-value, 0-fee"
(https://github.com/bitcoin/bitcoin/pull/27271)
ACK https://github.com/bitcoin/bitcoin/pull/27271/commits/d7cc503843769b789dc5f031b4ddc75d555ba980

Left a small grammar nit for the comment. Also, it would be really nice to have the `src/wallet/spend.cpp` change in one commit and the functional test change in a separate commit. Makes it much easier to verify that the functional test actually catches the bug, although not really a big deal here since the change is so small.

To test, I deleted your change to `src/wallet/spend.cpp` and verif
...

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