💬 andrewtoth commented on pull request "cli: add validation to cli side commands besides when it's used with -rpcwallet":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1127237286)
```suggestion
MULTIPLE_CLI_COMMAND_ERROR = "Error: you can only run one cli-command at a time, either {} or {}"
```

💬 jonatack commented on pull request "cli: add validation to cli side commands besides when it's used with -rpcwallet":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1127275368)
> Where does `bitcoin-cli -h` show how to solve this problem specifically?

`./src/bitcoin-cli -h | grep -A4 rpcwallet`

💬 theStack commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1127278080)
That looks dangerous -- generally I think we shouldn't ever call `.value()` on a std::optional object without checking for `.has_value()` first (similar as for and pointer dereferences), potentially risking a crash. In this case the GUI flow seems to ensure that the IP passed here is valid at an earlier stage (via the `ProxyAddressValidator`) so `LookupHost` _should_ always return a value, but I'd prefer to be rather safe than sorry.

💬 jonatack commented on pull request "cli: add validation to cli side commands besides when it's used with -rpcwallet":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1127282385)
The alternative is provided in the preceding sentence:

`Multiple wallets are loaded. Please select which wallet to use by requesting the RPC through the /wallet/<filename> URI path. Or for the CLI, specify the "-rpcwallet=<filename>" option before the command (run "bitcoin-cli -h" for help or "bitcoin-cli listwallets" to see which wallets are currently loaded).`

A good point is that the functional test would make it more clear if the full message is provided.

```diff
index 0f2e076fe8b.
...

💬 amitiuttarwar commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1127282542)
hm, this suggestion makes sense within the context of the first commit. however, the `search_tried` logic gets updated over the commits so it would be expanded anyways

💬 amitiuttarwar commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1127291828)
nice, done

💬 amitiuttarwar commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1127291922)
done

💬 amitiuttarwar commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#issuecomment-1457435139)
thanks for the review @brunoerg ! responded to all your feedback

💬 theStack commented on pull request "test: add coverage for sigop limit policy (`-bytespersigop` setting)":
(https://github.com/bitcoin/bitcoin/pull/27171#discussion_r1127302187)
The intention was to express the implication of exceeding the sig-op limit from a user's perspective (tx is treated as larger than it's serialized vsize => more fees needed to reach the same fee-rate; or as PR desc of #8365 put it "high-sigop transactions [...] need to pay a fee corresponding to the maximally-used resource."). Agree that the comment is rather misleading and not very helpful for the test, replaced it.

Generally struggled a bit with terminology for this PR (not even sure if tal
...

💬 S3RK commented on pull request "wallet: Turn `destdata` entries into `CAddressBookData` fields":
(https://github.com/bitcoin/bitcoin/pull/27215#issuecomment-1457734909)
Concept ACK

💬 MarcoFalke commented on pull request "wallet: Turn `destdata` entries into `CAddressBookData` fields":
(https://github.com/bitcoin/bitcoin/pull/27215#discussion_r1127555196)
nit: Returning a const value is not possible. You can either return a (mutable) value or a const reference. See also:


`�[1m/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/./wallet/wallet.h:230:5: �[0m�[0;1;31merror: �[0m�[1mreturn type 'const std::optional<std::pair<int64_t, std::string>>' (aka 'const optional<pair<long, basic_string<char>>>') is 'const'-qualified at the top level, which may reduce code readability without improving const correctness [readability-const
...

💬 fanquake commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1457806376)
ACKs here, but these two comments should be addressed:
> assuming this shouldn't use emplace_back https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1123317784 or avoid temporary vector allocations https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1123429117.

💬 MarcoFalke commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1127561494)
nit:

```
test/functional/wallet_labels.py:76:132: E703 statement ends with a semicolon

💬 MarcoFalke commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1127566183)
```suggestion
// Any other string is a programming error or disk corruption
```

nit: This could also happen on disk corruption, no?

💬 MarcoFalke commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1127564320)
```suggestion
} // no default case so the compiler will warn when a new enum is added

```

nit: typo

💬 fanquake commented on issue "Issue in `p2p_ibd_stalling.py` under Valgrind":
(https://github.com/bitcoin/bitcoin/issues/27208#issuecomment-1457879348)
> What did you set the timeout factor to?

I didn't change it at all. Will get you a log.

📝 MarcoFalke opened a pull request: "util: Work around ParseHex gcc cross compiler bug"
(https://github.com/bitcoin/bitcoin/pull/27218)
I fail to see how an explicit `ParseHex` template instantiation fails to also instantiate `TryParseHex`.

Nonetheless, to work around a compiler bug, change the explicit instantiation from `ParseHex` to `TryParseHex`. (`ParseHex` is inline anyway and will be instantiated by the compiler either way).

Fixes https://github.com/bitcoin/bitcoin/pull/25227#issuecomment-1456009757 :

```
CXXLD bitcoind
/usr/lib/gcc-cross/powerpc64le-linux-gnu/11/../../../../powerpc64le-linux-gnu/bin/ld: l
...

💬 MarcoFalke commented on pull request "util: Work around ParseHex gcc cross compiler bug":
(https://github.com/bitcoin/bitcoin/pull/27218#issuecomment-1457941463)
To test on a fresh install of Ubuntu/Debian with gcc-11 or later: https://github.com/bitcoin/bitcoin/pull/25227#issuecomment-1456215491

💬 MarcoFalke commented on pull request "Handle invalid hex encoding in ParseHex":
(https://github.com/bitcoin/bitcoin/pull/25227#issuecomment-1457953182)
> I guess it's having issues with the template for the std::optional<std::vector<uint8_t>> instantiation, since it's not available to the linker?

It might be a compiler bug where the compiler skips the explicit instantiation of a template that has a function body (and thus is already required to be implicitly instantiated)?

> Adding explicit instantiations should fix it, I think:

Thanks. Done something like that in #https://github.com/bitcoin/bitcoin/pull/27218

> Alternatively, I su
...

⚠️ 1440000bytes opened an issue: "`combinepsbt` RPC does not work with P2TR inputs"
(https://github.com/bitcoin/bitcoin/issues/27219)
[`combinepsbt`](https://bitcoincore.org/en/doc/24.0.0/rpc/rawtransactions/combinepsbt/) allows to distribute identical PSBTs to peers for signature and consolidate the resulting PSBT. However, this does not work with taproot UTXOs for me.

**Expected behavior**

Combined PSBT should return `'complete': True` if all inputs are signed in different PSBTs and finalized with `finalizepsbt`

**Actual behavior**

Unable to finalize the transaction that was combined from multiple signed PSBTs.

...