💬 stickies-v commented on pull request "rpc: Remove index-based Arg accessor":
(https://github.com/bitcoin/bitcoin/pull/29997#discussion_r1598417298)
Ah good point, I hadn't considered that the `Type` alias is introduced here. I think a preferable approach to me is to use the actual `typename` instead of aliasing it, such as in this diff. Optionally, I'd also be supportive of renaming `R` to something more descriptive such as `ReturnType`, but even with just `R` I think this is more clear:

<details>
<summary>git diff on fadb3eb57b</summary>

```diff
diff --git a/src/rpc/util.h b/src/rpc/util.h
index 9ba7fcf5e6..8fc598b873 100644
---
...

💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-2107482943)
Rebased on master.
Currently based on #30074 and #30078.
Dropped the commit related to `--ld-path`, and added a commit to remove binutils from the macOS Guix build env.

💬 ryanofsky commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1598423939)
re: https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1598392660

> Question: do we expect this to loop many times / under what circumstances? I tried adding logging to see what happens on my node, and I haven't seen it go more than once yet.

I think this is only intended to loop if there is an error loading, and the user is using a gui, and they are asked "Do you want to rebuild the block database now?" and they answer yes.

💬 cbergqvist commented on pull request "cli: Detect port errors in rpcconnect and rpcport":
(https://github.com/bitcoin/bitcoin/pull/29521#discussion_r1598426841)
`ToIntegral` itself returns a `std::optional` unfortunately so the suggested change won't compile as is Super-weird that this error popped up now.

💬 remyers commented on pull request "wallet: add coin selection parameter `add_excess_to_recipient_position` for changeless txs with excess that would be added to fees":
(https://github.com/bitcoin/bitcoin/pull/30080#issuecomment-2107502094)
Thanks for the feedback! I agree that it is worth considering how to make these parameters more useful for general users.

> `change_target` IIUC you want a specific min value for the change UTXO. Would a following alternative work? You provide your change address in the list of recipients with desired min amount and specify it as change UTXO using `change_position`parameter. The downside compared to a new option is that we always have a change output. Not sure if that's a problem in your case
...

💬 glozow commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1598432185)
Thanks @ryanofsky! That would explain why I didn't see it loop.

💬 Sjors commented on pull request "Intro: Have user choose assumevalid":
(https://github.com/bitcoin-core/gui/pull/149#issuecomment-2107510950)
Realistically anyone who wants to use this probably won't do so the first time they install the node. So they'll have to carefully delete the blocks and chainstate from their data dir, wipe the GUI settings and trigger this dialog again.

It seems both easier and safer to encourage such a user to add `reindex-chainstate=1` and `assumevalid=0` to their config file instead.

I don't think we should be cluttering the Intro screen for this, especially when looking ahead to Assume UTXO.

💬 maflcko commented on pull request "cli: Detect port errors in rpcconnect and rpcport":
(https://github.com/bitcoin/bitcoin/pull/29521#discussion_r1598437856)
I placed the `)` in the wrong place, I think. However, `value_or` exists and should be compile-able. See https://en.cppreference.com/w/cpp/utility/optional/value_or

💬 josibake commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#issuecomment-2107527748)
Concept ACK

Seems like a reasonable approach to the problem described in #29435 , will dig in more. Using a variant as a replacement for the Enum seems a bit odd at first glance and requires a lot of duplicate code for each struct. Perhaps another approach could be a class for `RemovalReason`, which contains the Enum as a field and then uses a std::variant to hold the data needed by the wallet, if any. Would be more extensible, and then the to string method could be defined once on the class
...

💬 maflcko commented on issue "Load PSBT error: Unable to decode PSBT":
(https://github.com/bitcoin/bitcoin/issues/30070#issuecomment-2107530835)
Are you able to reproduce in regtest, with exact steps to reproduce?

💬 TheCharlatan commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1598447607)
I'm not too concerned over the message being printed again, since there a whole bunch of log lines that get printed again too. The for loop should also only be executed once more, since if there is a failure on its second pass, the `reindex` option is true, making it return an `InitError`.

💬 glozow commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1598450015)
Thanks sgtm :+1: I didn't want to ignore the comment, but also wasn't sure how to test.

💬 maflcko commented on pull request "tests: improve wallet multisig descriptor test and docs":
(https://github.com/bitcoin/bitcoin/pull/29154#discussion_r1598465742)
> Done

Did you push the changes?

💬 fanquake commented on issue "upstream: GUIX closure contains too much unnecessary stuff":
(https://github.com/bitcoin/bitcoin/issues/30042#issuecomment-2107562656)
Related upstream change: https://git.savannah.gnu.org/cgit/guix.git/commit/?id=7b0f145802f0c2c785014293d748721678fef824.

💬 cbergqvist commented on pull request "cli: Detect port errors in rpcconnect and rpcport":
(https://github.com/bitcoin/bitcoin/pull/29521#discussion_r1598472302)
Yeah, that new version should work. Another possible variant.. but no way to know if the compiler becomes happier:
```suggestion
const uint16_t rpcport_int{ToIntegral<uint16_t>(rpcport_arg.value()).value_or(0)};
if (rpcport_int == 0) {
```

💬 fanquake commented on pull request "depends: sqlite 3.45.3":
(https://github.com/bitcoin/bitcoin/pull/29991#issuecomment-2107575009)
Not super pressing. I've been looking at ways we could build sqlite with CMake, given that will be one of the last Autotools holdouts. Some builders for the amalgamation exist, but are mostly based off/more usable with a more recent version.

💬 foolbear commented on issue "Load PSBT error: Unable to decode PSBT":
(https://github.com/bitcoin/bitcoin/issues/30070#issuecomment-2107575480)
keywords: not watch-only wallet, prune mode, importprivkey, migratewallet.

"And same procedure works fine in another wallet of test net", but I cannot make an address with UTXOs from years ago(maybe have something to do with prune mode?).

💬 maflcko commented on pull request "cli: Detect port errors in rpcconnect and rpcport":
(https://github.com/bitcoin/bitcoin/pull/29521#discussion_r1598483565)
> no way to know if the compiler becomes happier:

If it compiles locally, the code should be fine. If the CI warning persists, it should be disabled in the CI task config. (you can test by pushing, or running locally)

💬 Sjors commented on pull request "contrib: use ENV flags in get_arch":
(https://github.com/bitcoin/bitcoin/pull/30074#issuecomment-2107600543)
The second commit b59a027d957a4cffd225a681e6c85f9ae7fd77f3 seems trivially correct since `get_machine` is unused.

I got a bit confused about its history, so here it is:

1. It first became unused in #21255 (merged Feb 22, 2021)
2. It was then removed in #21428 (merged Mar 18, 2021)
3. It was reintroduced in #22381 (merged Jul 9, 2021) in order to skip RISCV in `TestSymbolChecks`
4. That skipping was no longer needed as of #27029 (merged Feb 17, 2023)

I'll do a guix build...

📝 willcl-ark opened a pull request: "rpc: move UniValue in blockToJSON"
(https://github.com/bitcoin/bitcoin/pull/30094)
Fixes: #24542

Without explicitly declaring the move, these `UniValues` get copied, causing increased memory usage. Fix this by explicitly moving the `UniValue` objects.

Used by `rest_block` and `getblock` RPC.