💬 kevkevinpal commented on pull request "test: Added test to ensure log and failure happen when work is less than active chainstate":
(https://github.com/bitcoin/bitcoin/pull/30105#issuecomment-2131273947)
> Just realised this is similar to this PR: #29428

Oh nice I didnt see this, closing this now

💬 kevkevinpal commented on pull request "test: Assumeutxo: snapshots with less work should not be loaded":
(https://github.com/bitcoin/bitcoin/pull/29428#issuecomment-2131274346)
utACK [df6dc2a](https://github.com/bitcoin/bitcoin/pull/29428/commits/df6dc2aaaeffc664006b86ee8c8797dc484ec40e)

💬 kevkevinpal commented on pull request "bench: enable wallet creation benchmarks on all platforms":
(https://github.com/bitcoin/bitcoin/pull/30122#issuecomment-2131286969)
utACK [7c8abf3](https://github.com/bitcoin/bitcoin/pull/30122/commits/7c8abf3c2001152423da06d25f9f4906611685ea)

🤔 tdb3 reviewed a pull request: "doc: update mention of generating bitcoin.conf"
(https://github.com/bitcoin/bitcoin/pull/30154#pullrequestreview-2079033825)
ACK for 9013e2b97e8f50d2be63ce740c42d0b0e0b9b7f2
Good find. Makes sense to point the user to conf file generation (helping to ensure the conf reflects the implementation). Followed the link (seemed to work fine).

💬 hebasto commented on issue "make cov fails with lcov-2":
(https://github.com/bitcoin/bitcoin/issues/28468#issuecomment-2131301980)
FWIW, the https://github.com/hebasto/bitcoin/pull/191 supports both LCOV versions for CMake.

💬 furszy commented on issue "Enable `importprivkey`, `addmultisigaddress` in descriptor wallets":
(https://github.com/bitcoin/bitcoin/issues/30175#issuecomment-2131302391)
I think `importaddress` and `addmultisigaddress` intents are clear enough and they map well to descriptors. Both specify the address type. Forcing users to learn how to craft a descriptor when they merely want to watch an address seems overwhelming to me. -> #27034 maps `importaddress` to `addr()` and `raw()` descriptors. And #28307 fixes and extends `createmultisig`/`addmultisigaddress`.

Not sure about `importprivkey` as it would clash with the new `void(KEY)` descriptor (#29136). The intent
...

👍 tdb3 approved a pull request: "cli: restrict multiple exclusive argument usage in bitcoin-cli"
(https://github.com/bitcoin/bitcoin/pull/30148#pullrequestreview-2079066296)
ACK for 628d2d4173660c1ad35b2d84524f71b92593d7cd

Thanks for picking this up. Built and ran all functional tests (all passed). Manually executed `bitcoin-cli` with one and multiple instances of the argument group covered by this PR. The error was seen (as expected) if more than one argument of the group was specified.

Left a few notes.

💬 tdb3 commented on pull request "cli: restrict multiple exclusive argument usage in bitcoin-cli":
(https://github.com/bitcoin/bitcoin/pull/30148#discussion_r1614742727)
The implicit conversions from bool to int seem safe (at least at first glance, https://en.cppreference.com/w/cpp/language/implicit_conversion), but I didn't get a change to dig into C++ standard docs to check. It is a tiny bit unintuitive for a reader, but this doesn't seem too cryptic and seems cleaner than the separate `if` statements and temp variable used in 27815.

💬 tdb3 commented on pull request "cli: restrict multiple exclusive argument usage in bitcoin-cli":
(https://github.com/bitcoin/bitcoin/pull/30148#discussion_r1614742124)
What's the rationale for using `IsArgSet()` for `getinfo` but `GetBoolArg()` for the others? Maybe I'm overlooking something simple.

Tried the following locally and didn't see any issues with it:
```diff
diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index 4002424af0..b682544fa2 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -1175,10 +1175,11 @@ static int CommandLineRPC(int argc, char *argv[])
fputc('\n', stdout);
}
}
+
...

💬 maflcko commented on pull request "test: switch from curl to urllib for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#discussion_r1614761341)
Was this addressed?

💬 maflcko commented on pull request "doc, rpc: Release notes and follow-ups for #29612":
(https://github.com/bitcoin/bitcoin/pull/30167#discussion_r1614770544)
I agree that the assert should be safe here, but I think consistency-wise it makes sense to use exceptions (even for internal logic errors) over asserts/assumes in contexts where exceptions are already used and where it is safe to use them. RPC and serialization are two such areas.

Thanks for fixing.

👍 maflcko approved a pull request: "doc, rpc: Release notes and follow-ups for #29612"
(https://github.com/bitcoin/bitcoin/pull/30167#pullrequestreview-2079083069)
utACK efc1b5be8a4696c0db19ba18316b2d4ed09e10f2

👍 AngusP approved a pull request: "test: switch from curl to urllib for HTTP requests"
(https://github.com/bitcoin/bitcoin/pull/29970#pullrequestreview-2079184785)
ACK 588cad38f7813d88f022d0304aa20a0b96d184ed

⚠️ lcharles123 opened an issue: "Add "maxuploadtargettimeframe" to change the timeframe considered by "maxuploadtarget""
(https://github.com/bitcoin/bitcoin/issues/30176)
### Please describe the feature you'd like to see added.

The "maxuploadtarget"
# Tries to keep outbound traffic under the given target per 24h (one day).

But many ISPs that does not have unlimited traffic account it by month. So it will be a handy setting to avoid the needing to calculate the amount transferred by the billing period. This new setting can be specified in days.

### Is your feature related to a problem, if so please describe it.

_No response_

### Describe the solution you'
...

💬 sipa commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#issuecomment-2131728449)
Apparently `std::is_trivially_default_constructible_v<T>` does not imply you can just memset 0 to construct the objects (or at least, I can't find evidence of that). So I've dropped that branch.

💬 ajtowns commented on pull request "doc: note that you can assume C++20.":
(https://github.com/bitcoin/bitcoin/pull/30136#discussion_r1614990655)
> "you can assume c++20"

Note that it depends on [compiler support](https://en.cppreference.com/w/cpp/compiler_support/20) for the individual features; for instance [modules](https://en.cppreference.com/w/cpp/language/modules) are nominally part of C++20, but still have pretty poor compiler support, so aren't something we can make use of. Conversely, sometimes if a feature is widely supported by compilers/libs, we'll use it even if it's not part of the current language standard (eg #24531 #25
...

💬 laanwj commented on pull request "doc: note that you can assume C++20.":
(https://github.com/bitcoin/bitcoin/pull/30136#discussion_r1615128529)
Right, in practice not even all C++11 features are fully supported, nor always desirable, e.g. don't `thread_local` unless it's for a very good reason and only for POD types. But as starting point before listing all the exceptions i think it still makes sense to mention.

💬 Amarrufo-1 commented on pull request "doc: note that you can assume C++20.":
(https://github.com/bitcoin/bitcoin/pull/30136#issuecomment-2132135446)
I don't even know wat that means

On Sun, May 26, 2024, 1:24 AM laanwj ***@***.***> wrote:

> ***@***.**** commented on this pull request.
> ------------------------------
>
> In doc/developer-notes.md
> <https://github.com/bitcoin/bitcoin/pull/30136#discussion_r1615128529>:
>
> > @@ -771,6 +771,8 @@ Wallet
> General C++
> -------------
>
> +As of v27.0, we require a compiler which meets at least the C++20 standard.
>
> Right, in practice not even all C++11 features are fully s
...

💬 maflcko commented on pull request "doc: add guidance for RPC to developer notes":
(https://github.com/bitcoin/bitcoin/pull/30142#discussion_r1615135575)
how is this different from "- Try to make the RPC response a JSON object."?

The `"warnings"` example here doesn't make much sense, because both are equally flexible to expansion. (See the plural `s`.) The reason why array should be preferred over string is that array is the correct type to represent an array. (But that is obvious, isn't it?)

⚠️ foolbear opened an issue: "show error "could not sign any more inputs" when sign PSBT for multisig"
(https://github.com/bitcoin/bitcoin/issues/30177)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

show error "could not sign any more inputs" when sign PSBT for multisig

### Expected behaviour

will sign succ and broadcast.

1. testnet
2. descriptor wallet
3. [create 2/3 multisig wallet and operation](https://github.com/bitcoin/bitcoin/blob/master/doc/multisig-tutorial.md)
4. succ if using walletprocesspsbt/sendrawtransaction RPC

### Steps to reproduce

1. create unsigned from a
...