💬 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)`

💬 ajtowns commented on pull request "RFC: Instanced logs":
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2194940918)
> this will allow us to instantiate a per-context logger, which is necessary for the kernel.

I don't understand where this is coming from. Libraries can export globals, eg `stdin`, so I don't see why you're claiming it's necessary for the kernel when it's not even necessary for libc.

🤔 instagibbs reviewed a pull request: "cluster mempool: cluster linearization algorithm"
(https://github.com/bitcoin/bitcoin/pull/30126#pullrequestreview-2116172410)
a couple general comments:

1) The bespoke DepGraphFormatter is pretty artisanal and as a reviewer I'm relying heavily on the cluster->depgraph->check depgraph matches -> serialization round-trip for asserting correctness. Unit tests-as-documentation is one thing I'm kind of missing, especially with the more involved parts.
2) Matching the actual LIMO commit with the LIMO literature/theory available is difficult for me(e.g., what's sufficient vs necessary). I know there's a new writeup coming
...

💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1638465834)
Should we `Assume()`/short-circuit if `parent == child`

💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1640177227)
could leave an assert to this effect right after this

💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1638438741)
`Cluster<TestBitSet> cluster(num_tx, std::make_pair(FeeFrac{0, 1}, TestBitSet()));` should work?

💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1638472774)
Where does the amortization come in?

💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1644863011)
```Suggestion
Assume(!m_ancestor_set_feerates[first].IsEmpty());
anc_feerate = m_ancestor_set_feerates[first];
```

💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1640177309)
could leave an assert to this effect right after this

💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1638488625)
```Suggestion
LIMITED_WHILE(provider.remaining_bytes() > 0, TestBitSet::Size() * TestBitSet::Size()) {
```

💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1638450101)
```Suggestion
// Read (parent, child) pairs, and add them to the cluster and depgraph.
```

💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1644945090)
it'd be worth it to have `del_set` be sometimes overcomplete, including random subsets of `~todo` which should be handled internally by being dropped. Alternatively it could be disallowed via `Assume()`?

💬 ajtowns commented on issue "RFC: Misused LogError and LogWarning macros":
(https://github.com/bitcoin/bitcoin/issues/30348#issuecomment-2194982649)
> I just don't see a significant distinction between different syntaxes `LogDebug(...)` or `LogPrint(BCLog::Debug, ...)` or even `log.Debug(...)` so have not been trying to raise syntax as an issue.

The second one is an extra 13 characters of noise each time (20 if you don't do something to avoid having to type `BCLog::Level::Debug`); for the third one, C++ doesn't allow you to have it be a magic macro where the arguments aren't even evaluated when debug logging isn't enabled. Far better to h
...

📝 Sjors opened a pull request: "[WIP] libbitcoin_net"
(https://github.com/bitcoin/bitcoin/pull/30350)

✅ Sjors closed a pull request: "[WIP] libbitcoin_net"
(https://github.com/bitcoin/bitcoin/pull/30350)