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

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

Oh that makes sense. If you think it would be an improvement, I could change the patch command to `echo '...' | tee >(patch -p1)` so the patch is copied to stdout

💬 maflcko commented on pull request "ci: remove unnecessary git diff after applying patch":
(https://github.com/bitcoin/bitcoin/pull/29461#issuecomment-1957099158)
Sure, anything is fine here.

💬 kevkevinpal commented on pull request "test: check_mempool_result negative feerate":
(https://github.com/bitcoin/bitcoin/pull/29459#discussion_r1497855816)
oops fixed in [bf264e0](https://github.com/bitcoin/bitcoin/pull/29459/commits/bf264e05981e3809715f34f548138d53991db6f2)

💬 maflcko commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#issuecomment-1957135673)
gh-sync

💬 maflcko commented on pull request "test: check_mempool_result negative feerate":
(https://github.com/bitcoin/bitcoin/pull/29459#issuecomment-1957144550)
lgtm ACK bf264e05981e3809715f34f548138d53991db6f2

💬 fjahr commented on pull request "RPC: access RPC arguments by name":
(https://github.com/bitcoin/bitcoin/pull/29277#issuecomment-1957172564)
> I doubt this will have any impact. It could even perform worse, and require more code?

Same amount of code and I don't see how this would perform worse: https://github.com/fjahr/bitcoin/commit/cbda2cf2c012030544ffc03620cbfcb91b1984be I also find this easier to reason about.

> Also, maps aren't constexpr, so if this is moved toward constexpr/consteval, it will have to be re-written again.

I guess I don't have enough insight into future development plans of the RPC to judge that if is.
...

💬 ryanofsky commented on pull request "ci: avoid running git diff after patching":
(https://github.com/bitcoin/bitcoin/pull/29461#issuecomment-1957175791)
Updated c835176774a4803132343d5e096cdc7fe902e8eb -> 84388c942cb035fed546eda360ae6b40c6cfac09 ([`pr/nodiff.1`](https://github.com/ryanofsky/bitcoin/commits/pr/nodiff.1) -> [`pr/nodiff.2`](https://github.com/ryanofsky/bitcoin/commits/pr/nodiff.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/nodiff.1..pr/nodiff.2)) restoring diff output with tee

💬 fjahr commented on pull request "contrib: add test for bucketing with asmap":
(https://github.com/bitcoin/bitcoin/pull/28869#issuecomment-1957241295)
tACK facfc2676b95949fa814a7686334d85d99e52519

CI failure seems unrelated...

💬 maflcko commented on pull request "RPC: access RPC arguments by name":
(https://github.com/bitcoin/bitcoin/pull/29277#issuecomment-1957248978)
> Same amount of code and I don't see how this would perform worse: [fjahr@cbda2cf](https://github.com/fjahr/bitcoin/commit/cbda2cf2c012030544ffc03620cbfcb91b1984be) I also find this easier to reason about.

You removed the `CHECK_NONFATAL` to mark the bug as an internal bug.

> which comes out to O(n^2)

The `O` notation only makes sense for large `n`. Is there more than one RPC with more than n=3? Again, if you measure it, I wouldn't be surprised if the runtime is slower for a map. Thoug
...