💬 paplorinc commented on pull request "WIP: Simplify SipHash":
(https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1657205524)
Thank you, I've pushed it in https://github.com/bitcoin/bitcoin/pull/30349

💬 maflcko commented on pull request "refactor: Improve SipHash_32b accuracy to avoid potential optimization issues":
(https://github.com/bitcoin/bitcoin/pull/30349#issuecomment-2194807648)
Changing the behavior of something is not a refactor.

💬 jonatack commented on issue "RFC: Misused LogError and LogWarning macros":
(https://github.com/bitcoin/bitcoin/issues/30348#issuecomment-2194832230)
@ryanofsky all those unnecessary macros add complexity and more ducks to keep in a row -- for no benefit.

One primary benefit I hope to see from the `trace` level is to be able to separate out very verbose, low-level, high-frequency logging. With `net` category logging, for instance, to use `debug` for high-level, lower-frequency events at the peer level, and `trace` for low-level, very high frequency events at the p2p messages level.

👍 pablomartin4btc approved a pull request: "Show maximum mempool size in information window"
(https://github.com/bitcoin-core/gui/pull/825#pullrequestreview-2145552451)
tACK 4a028cf54c0502bc9ba0804bf1ae413b20a007cb


💬 ismaelsadeeq commented on pull request "Wallet: Add `max_tx_weight` to transaction funding options (take 2)":
(https://github.com/bitcoin/bitcoin/pull/29523#discussion_r1657231561)
fixed

💬 ismaelsadeeq commented on pull request "Wallet: Add `max_tx_weight` to transaction funding options (take 2)":
(https://github.com/bitcoin/bitcoin/pull/29523#discussion_r1657231916)
Yes, removed

💬 ismaelsadeeq commented on pull request "Wallet: Add `max_tx_weight` to transaction funding options (take 2)":
(https://github.com/bitcoin/bitcoin/pull/29523#discussion_r1657232237)
removed

💬 ismaelsadeeq commented on pull request "Wallet: Add `max_tx_weight` to transaction funding options (take 2)":
(https://github.com/bitcoin/bitcoin/pull/29523#discussion_r1657233836)
added the line back, thanks

💬 ismaelsadeeq commented on pull request "Wallet: Add `max_tx_weight` to transaction funding options (take 2)":
(https://github.com/bitcoin/bitcoin/pull/29523#discussion_r1657236943)
Yes it's not needed.
I removed this commit
This can come as a separate PR, the size output can also be added to the other funding RPC's?

💬 ajtowns commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1657237193)
I don't think it makes sense for us to have 7 levels of logging -- even 5 is a lot. Renaming Warning to Critical and Error to Fatal for the reasons given seems fine, but the old names shouldn't be kept around in that case.

💬 theuni commented on pull request "kernel: remove mempool_persist":
(https://github.com/bitcoin/bitcoin/pull/30344#discussion_r1657238803)
Done.

💬 theuni commented on pull request "kernel: remove mempool_persist":
(https://github.com/bitcoin/bitcoin/pull/30344#issuecomment-2194854058)
> I've got this on a branch that I'll PR as soon as #30141 is merged: https://github.com/TheCharlatan/bitcoin/commits/kernelRmGlobals/ . I try to keep the open PRs limited to get more focused review.

Aha, I see.

I think this one is simple and self-contained enough to go in on its own unless you're opposed.

I'll ping you on these in the future to make sure you don't already have them teed up.

Also, concept ACK on your next branch. Will give #30141 a final review and ACK to keep things
...

💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1657242662)
unrelated nit: (one line above) `insecure_GetRandHash` was replaced years ago by `InsecureRand256`. Can ideally be fixed up in a follow-up.

💬 ismaelsadeeq commented on pull request "Wallet: Add `max_tx_weight` to transaction funding options (take 2)":
(https://github.com/bitcoin/bitcoin/pull/29523#discussion_r1657251451)
Yes, good catch.
Updated

💬 ismaelsadeeq commented on pull request "Wallet: Add `max_tx_weight` to transaction funding options (take 2)":
(https://github.com/bitcoin/bitcoin/pull/29523#issuecomment-2194909310)


Thanks for the reviews @furszy @rkrux

Force-pushed from b3ac1179ff90fe261af4e6dc9c7af64e1518b435 to d8febf6d9ea018cf8e980ee036fb5ccd8ea8887a
Compare diff [b3ac1179ff..734076c6d](https://github.com/bitcoin/bitcoin/compare/b3ac1179ff90fe261af4e6dc9c7af64e1518b435..734076c6de1781f957c8bc3bf7ed6951920cfcf6)

---

Changes:
- Rebased after [PR 30309](https://github.com/bitcoin/bitcoin/pull/30309) and updated the maximum transaction weight check to use the user passed value when given or
...

💬 darosior commented on pull request "fuzz: bound some miniscript operations to avoid fuzz timeouts":
(https://github.com/bitcoin/bitcoin/pull/30197#issuecomment-2194915751)
Rebased on master to take advantage of #30229.

💬 ajtowns commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2194926577)
> Rereading AJ's comments, I'm not sure this paragraph actually represents what he thinks, so striking it out.

Thanks for that, it was bumming me out thinking about how to try and clarify it.

I really prefer your approach in #30348 and #30347 where you identify a specific objective problem and separately propose a targeted fix for exactly that problem.

> logging options (before and after this PR) actually do not support filtering out Info/Warning/Error messages at all, by category or ot
...

💬 virtu commented on pull request "contrib: asmap-tool - Compare ASMaps with respect to specific addresses":
(https://github.com/bitcoin/bitcoin/pull/30246#discussion_r1657278263)
Moving `sys` doesn't fully fix the import block because it is separated from the next function definition by only one newline. If I added another newline below the import block fix it, I should probably also add additional newlines between function definitions for consistency's sake. Not sure if any/how much refactoring should be included here. wdyt?

💬 ryanofsky commented on issue "RFC: Misused LogError and LogWarning macros":
(https://github.com/bitcoin/bitcoin/issues/30348#issuecomment-2194927972)
re: https://github.com/bitcoin/bitcoin/issues/30348#issuecomment-2194832230

> @ryanofsky all those unnecessary macros add complexity and more ducks to keep in a row -- for no benefit.

I agree with that and your other points. I particularly agree the current implementation of the macros is complicated and inconsistent, and I wrote #29256 to make all the macros accept the same parameters and use the [same implementation](https://github.com/ryanofsky/bitcoin/blob/38d3298ea7e547797871140a02df3
...

💬 virtu commented on pull request "contrib: asmap-tool - Compare ASMaps with respect to specific addresses":
(https://github.com/bitcoin/bitcoin/pull/30246#discussion_r1657285832)
Good idea. Total change is now broken down into migrations (from one AS to another), assignments (previously unassigned, now assigned to an AS) and unassignments (vice versa).

`Summary: 371 (1.37%) of 27,050 addresses were reassigned (migrations=242, assignments=125, unassignments=4)`