💬 maflcko commented on pull request "ci: remove unnecessary git diff after applying patch":
(https://github.com/bitcoin/bitcoin/pull/29461#issuecomment-1956954836)
It is there to clarify the CI is using different code than the one in the source dir.

lgtm ACK c835176774a4803132343d5e096cdc7fe902e8eb

👍 maflcko approved a pull request: "test: Handle functional test disk-full error"
(https://github.com/bitcoin/bitcoin/pull/29335#pullrequestreview-1893536162)
lgtm

💬 maflcko commented on pull request "test: Handle functional test disk-full error":
(https://github.com/bitcoin/bitcoin/pull/29335#discussion_r1497794666)
```suggestion
# Exit the test if there is not enough space on the testing dir

if shutil.disk_usage(tmpdir).free < MIN_FREE_SPACE:
```

💬 maflcko commented on pull request "test: Handle functional test disk-full error":
(https://github.com/bitcoin/bitcoin/pull/29335#discussion_r1497794980)
nit: can just inline this?

👍 theStack approved a pull request: "test: assert rpc error for addnode v2transport not enabled"
(https://github.com/bitcoin/bitcoin/pull/29460#pullrequestreview-1893543293)
Code-review ACK 345169a7523f00209985da88e0171e8687589c25

🚀 fanquake merged a pull request: "test: assert rpc error for addnode v2transport not enabled"
(https://github.com/bitcoin/bitcoin/pull/29460)

💬 fanquake commented on pull request "RFC: build: remove confusing and inconsistent disable-asm option":
(https://github.com/bitcoin/bitcoin/pull/29407#issuecomment-1956977378)
> @fanquake Do you think it's too late for this for v27?

I think it's still possible to do this for v27. It's not clear to me that the other change is strictly a requirement for doing this change?, but I also think that could go in.

💬 luke-jr commented on pull request "RPC/Wallet: Add "use_txids" to output of getaddressinfo":
(https://github.com/bitcoin/bitcoin/pull/22693#issuecomment-1956989002)
Rebased again

💬 glozow commented on pull request "policy: enable sibling eviction for v3 transactions":
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1497818462)
You're right, it's not necessary. I think all we need is to gate on `ATMPArgs::m_allow_replacement` to not hit it in a multi-testmempoolaccept.

💬 ryanofsky commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1496381717)
re: https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1486106049

> In commit [b8900ad](https://github.com/bitcoin/bitcoin/commit/b8900adcd232bcb2b7475be19b5457c91052ab5a): Need to slightly adjust the comment here, how about: `* @return true if we successfully combined the transactions, false if they were not compatible`

Thanks, used suggestion and tweaked it to match other bool returns in this file

💬 ryanofsky commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1496381604)
re: https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1485612372

> There is one `wallet` too many here.

Good catch, fixed

💬 ryanofsky commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1497801368)
re: https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1486097145

> In commit [b8900ad](https://github.com/bitcoin/bitcoin/commit/b8900adcd232bcb2b7475be19b5457c91052ab5a): Can you either add a line about its removal in the commit message, or do it in a separate commit?

Thanks, I added a note about dropping unused error codes in the commit message

🤔 ryanofsky reviewed a pull request: "kernel: Streamline util library"
(https://github.com/bitcoin/bitcoin/pull/29015#pullrequestreview-1891396396)
Thanks for the reviews!

Updated 7ef33b2bad71a230428b653d6d0297df49630d99 -> 486379c10fdce568a64b78cbb2dad1fa52263a79 ([`pr/rmutil.9`](https://github.com/ryanofsky/bitcoin/commits/pr/rmutil.9) -> [`pr/rmutil.10`](https://github.com/ryanofsky/bitcoin/commits/pr/rmutil.10), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/rmutil.9..pr/rmutil.10)) with suggestions

🤔 t-bast reviewed a pull request: "wallet: Target a pre-defined utxo set composition by adjusting change outputs"
(https://github.com/bitcoin/bitcoin/pull/29442#pullrequestreview-1893495240)
I really like the fact that it consists mostly of a pre-processing step before coin selection runs, followed by a post-processing step on the output of coin selection.

Should we also tweak which coin selection algorithms are run depending on whether we're trying to refill buckets (and how aggressive we'd like to be) or not? Some of them may not be well suited for that?

💬 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#discussion_r1497769911)
Loading this file every time we fund a transaction doesn't seem reasonable. I think we should start with a static file that needs a restart whenever the node operator wants to change values, and we can later decide how to make this more dynamic (if it's even necessary).

💬 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#discussion_r1497828903)
I'm wondering whether we should use a fully deterministic algorithm to split the change into buckets, I'm afraid this is exactly the kind of algorithm that may end up in a loop where one funding attempt does something, and the next one undoes it. It feels like adding randomization may be the simple way to avoid this? But maybe it's too early in the algorithm design phase to decide.

💬 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#discussion_r1497820248)
I'm not sure this is really what we want. I think this function should try to refill multiple buckets at once (when amounts allow it), not just the most depleted one:

- Check which utxo buckets should be refilled:
- Initialize a `to_refill` list of `(bucket, target_quantity)`
- For each utxo bucket:
- If the feerate is low (below a to-be-defined threshold) and the bucket is less than 70% full (or a to-be-defined threshold that could be configurable):
- Add this bucket to `to
...

💬 sipa commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#issuecomment-1957073882)
utACK 40f505583f4edeb2859aeb70da20c6374d331a4f

✅ sipa closed a pull request: "serialization: Support for multiple parameters"
(https://github.com/bitcoin/bitcoin/pull/28929)

📝 sipa reopened a pull request: "serialization: Support for multiple parameters"
(https://github.com/bitcoin/bitcoin/pull/28929)
Currently it is only possible to attach one serialization parameter to a stream at a time. For example, it is not possible to set a parameter controlling the transaction format and a parameter controlling the address format at the same time because one parameter will override the other.

This limitation is inconvenient for multiprocess code since it is not possible to create just one type of stream and serialize any object to it. Instead it is necessary to create different streams for differen
...