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

✅ maflcko closed an issue: "Restore wallet taking forever to load"
(https://github.com/bitcoin/bitcoin/issues/30108)

💬 maflcko commented on pull request "build, test, doc: Temporarily remove Android-related stuff":
(https://github.com/bitcoin/bitcoin/pull/30049#issuecomment-2132188239)
concept ACK

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

💬 pythcoiner commented on issue "rpc: actually deprecate `rpcuser` & `rpcpass`":
(https://github.com/bitcoin/bitcoin/issues/29240#issuecomment-2132223951)
> Asking users who have always been able to just run the binaries + a config file, to now run a python script from `share/` to generate auth credentials feels a bit, messy?
> * Perhaps `bitcoin-cli` should get a `generaterpcauth` command?

good point, especially for windows users that get the binaries from `bitcoin.org`, they get only the `.exe`, i don't know if they have access to `/share` after installation?
Maybe a feature in bitcoin-qt is also needed?

💬 ceffikhan commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#issuecomment-2132341237)
> follow



> In the functional tests there are lots of cases where we assert != which we now swap with assert_not_equal to be more readable
>
> This is motivated/uses logic from this PR which was closed #28528 This partially helps #23119
>
> I've broken it up to just `assert_not_equal` to keep the PR smaller as suggested in [#28528 (comment)](https://github.com/bitcoin/bitcoin/pull/28528#issuecomment-1959945805)
>
> I can create follow up PR's if this is wanted

cdcd