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

💬 willcl-ark commented on pull request "rpc: move UniValue in blockToJSON":
(https://github.com/bitcoin/bitcoin/pull/30094#issuecomment-2107608396)
Before and after of RSS:


💬 maflcko commented on pull request "rpc: move UniValue in blockToJSON":
(https://github.com/bitcoin/bitcoin/pull/30094#issuecomment-2107608765)
review ACK b77bad309e92f176f340598eec056eb7bff86f5f

Should be fine either way, but did you also test it?

💬 willcl-ark commented on issue "Slow memory leak in v22.0?":
(https://github.com/bitcoin/bitcoin/issues/24542#issuecomment-2107609517)
> Can you explain this? It has a move constructor, ...

I think you are correct here, the compiler implicitly generates move-contructors which are enough to fix this issue, if combined with appropriate `std::move`. I did not think that was the case, but I was using a non-reproducible test. I made my test reproducible and tested:

1. v27.0
2. addition of `std::move` only
3. addition of `std::move` + excplicit move constructor(s)
4. addition of `std::move` + excplicit move constructor(s) +
...

💬 setavenger commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1598499179)
>
> I should add that I was using `IsDust()` with `DUST_RELAY_TX_FEE`, which takes SegWit into account to decide the exact dust limit.
>

Ah I'm not sure how this should affect the results tbh. In my index I just looked at the UTXO value, no modifications.

>
> I looked at the largest output, not all outputs.

Yeah but my current understanding is that in order to see no difference on the index size at all, all of those UTXOs must have been in combination with bigger UTXOs, not eligib
...

💬 TheCharlatan commented on pull request "contrib: use ENV flags in get_arch":
(https://github.com/bitcoin/bitcoin/pull/30074#issuecomment-2107613473)
Guix build (aarch64):
```
fcb75f2a3b61befeabe143301e5db490d23b1ee4a5edb109f219dea575395b49 guix-build-b59a027d957a/output/aarch64-linux-gnu/SHA256SUMS.part
15310e3aff2a9ef71bad06315fcd425e56611d5bed7f6c394f3fe2248140e17b guix-build-b59a027d957a/output/aarch64-linux-gnu/bitcoin-b59a027d957a-aarch64-linux-gnu-debug.tar.gz
8e60146d39a47e9c332e3b842e239f1ca7773ca2e6788876155377f4568ab1fa guix-build-b59a027d957a/output/aarch64-linux-gnu/bitcoin-b59a027d957a-aarch64-linux-gnu.tar.gz
10432256ea
...