💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1494213677)
(though I think the specific reason here was because I was in a hurry)

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1494217939)
Brought it back.

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1494226737)
Done

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-1952006804)
Applied @fjahr's patches and running the indexer again...

💬 maflcko commented on issue "getJsonToken assumes underlying string is null-terminated but requires end pointer":
(https://github.com/bitcoin/bitcoin/issues/28260#issuecomment-1952037763)
> My changes have a few orthogonal, but related fixes and test additions. We could make different pull-requests for them. Let me know what you prefer.

According to the dev notes, they should be different commits. Ideally bugfixes and features in a different PR. See https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#committing-patches (The doc, and it's links to the dev notes and productivity notes should explain all your questions)

💬 maflcko commented on issue "[CI/CD]Release channels?":
(https://github.com/bitcoin/bitcoin/issues/29446#issuecomment-1952055714)
Yes, they already exist. I presume "canary" means nightly, which is the `master` branch. Otherwise, you can compile from `rc`s (release candidates), which are the "beta". Finally, if you compile from the latest tag, it can be considered "stable".

💬 recursive-rat4 commented on issue "Proposal: Adopt simple SPDX-License-Identifier Across Bitcoin Codebase":
(https://github.com/bitcoin/bitcoin/issues/29445#issuecomment-1952060401)
>version control already preserves historical data and legal integrity

Version control history is often not preserved in practice for whatever reasons. A fresh example of this is https://github.com/testng-team/testng-ant splitted up in https://github.com/testng-team/testng/issues/3033

💬 maflcko commented on pull request "rpc: Fixed signed integer overflow for large feerates":
(https://github.com/bitcoin/bitcoin/pull/29434#discussion_r1494273142)
Sure, may do if I re-touch.

💬 fanquake commented on issue "test: Write assumeutxo tests":
(https://github.com/bitcoin/bitcoin/issues/28648#issuecomment-1952071633)
There are more PRs open. i.e #29428.

🤔 Sjors reviewed a pull request: "test: add end-to-end tests for CConnman and PeerManager"
(https://github.com/bitcoin/bitcoin/pull/26812#pullrequestreview-1888057050)
Reviewed the fist commit bee6bdf0a5084b2d5749ff06ad63c0e77816c733. Added a question for the second.

💬 Sjors commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1494319538)
f42e4f3b3b4b3ca1945d1ea298b443f1cecaf2ea: this only works with v1 transport. In both v2 transport and the (proposed) Stratum v2 transport (#29432) bytes on the wire need further processing to reconstruct the underlying message.

For those it's more useful to have a `GetBytes(size_t n)` helper method that waits until `n` bytes have been received.

(Specifically for the Stratum v2 I'm also trying to completely avoid a dependency on `CNetMessage`, but the design for that is still in flux)

💬 hebasto commented on issue "depends: `touch` command for creating determinstic archive timestamps fails on OpenBSD":
(https://github.com/bitcoin/bitcoin/issues/29447#issuecomment-1952130268)
> Haven't investigated deeper what `-h` exactly does

```
$ touch --help | grep -A 2 -e "-h,"
-h, --no-dereference affect each symbolic link instead of any referenced
file (useful only on systems that can change the
timestamps of a symlink)
```

💬 t-bast commented on pull request "wallet: Target a pre-defined utxo set composition by adjusting change outputs":
(https://github.com/bitcoin/bitcoin/pull/29442#issuecomment-1952151548)
@murchandamus we'd love to get your feedback on this! I'll try to summarize at a higher level what we'd like to achieve.

The `bitcoind` wallet currently tries to keep a somewhat "minimal" utxo set and actively consolidates user utxos, because it assumes that we receive transactions as much (or more) than we send transactions and wants outgoing transactions to be as cheap as possible. But that's not true of liquidity service providers, who usually only receive funds when refilling the wallet f
...

💬 naumenkogs commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#discussion_r1494332423)
Might as well fix ` if (new_count + tried_count == 0) return {};` above. While it seems like a bug in the existing code, it's really hard to trigger.... feeding addrman with that much garbage would be a failure probably before the size_t limit is hit :)

💬 naumenkogs commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#discussion_r1494214557)
3f99f5a6e58fae4981228929045ed7be32463381

Might be worth making this overflow-safe?

💬 dergoegge commented on pull request "[26.1] final changes for 26.1rc1":
(https://github.com/bitcoin/bitcoin/pull/29440#issuecomment-1952185126)
Fuzz job times out because https://github.com/bitcoin/bitcoin/pull/28933 has not been back-ported

⚠️ maflcko opened an issue: "ci_native_asan: "
(https://github.com/bitcoin-core/gui/issues/796)
Not sure if it is reproducible, but the CI task failed locally.

Commit: baed5edeb611d949982c849461949c645f8998a7

Log:

```
QDEBUG : AppTests::appTests() requestInitialize : Requesting initialize
QDEBUG : AppTests::appTests() Running initialization in thread
QDEBUG : AppTests::appTests() initializeResult : Initialization result: true
QINFO : AppTests::appTests() Platform customization: "other"
QWARN : AppTests::appTests() This plugin does not support propagateSizeHints()
QWARN :
...

💬 maflcko commented on pull request "[26.1] final changes for 26.1rc1":
(https://github.com/bitcoin/bitcoin/pull/29440#issuecomment-1952199738)
Fuzz will fail either way, because the banman fuzz target isn't updated either to include the fix.

```
fuzz: test/fuzz/banman.cpp:112: void banman_fuzz_target(FuzzBufferType): Assertion `banmap == banmap_read' failed.

💬 maflcko commented on pull request "RPC: access RPC arguments by name":
(https://github.com/bitcoin/bitcoin/pull/29277#issuecomment-1952207658)
> @stickies-v have you considered preparing a map once when an RPC is called rather than doing the search for the index every time an arg is accessed?

I doubt this will have any impact. It could even perform worse, and require more code? Also, maps aren't constexpr, so if this is moved toward constexpr/consteval, it will have to be re-written again.

💬 willcl-ark commented on pull request "rpc: Drop migratewallet experimental warning":
(https://github.com/bitcoin/bitcoin/pull/28037#issuecomment-1952211224)
@achow101 for context my test wallet has 8016 descriptors after migration:

```fish
will@will-ubuntu in ~ : 🐍 3.11.6
₿ /home/will/src/bitcoin/src/bitcoin-cli -regtest -datadir=/media/will/PEPSI/regtest-v60000 listdescriptors | jq '.descriptors | length'
8016
```

I have also in the meantime tested a branch from @furszy which reduced migration time from 30 mins to 80 seconds 👀