Bitcoin Core Github
44 subscribers
121K links
Download Telegram
🤔 maflcko reviewed a pull request: "kernel: De-globalize static validation variables"
(https://github.com/bitcoin/bitcoin/pull/30425#pullrequestreview-2177390369)
ACK 51fa26239af9bbfd44029aaf595cb4c6a8d4a75d 🍚

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 51fa26239af9bbfd44029aaf59
...
💬 ismaelsadeeq commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1677594301)
nit: I think this is obvious
```suggestion
```
💬 ismaelsadeeq commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1677650738)
maybe just return early in that case and avoid the looping?
💬 ismaelsadeeq commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1677652342)
Should we return early if dependency already exist ?
💬 ismaelsadeeq commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1677606491)
nit: be explicit we are adding direct parents
```suggestion
for (ClusterIndex i = 0; i < cluster.size(); ++i) {
// Fill in fee, size
entries[i].feerate = cluster[i].first;
// Fill in direct parents as ancestors
entries[i].ancestors = cluster[i].second;
// Make sure transactions are ancestors of themselves.
entries[i].ancestors.Set(i);
}
```
🤔 fjahr reviewed a pull request: "refactor: Improve assumeutxo state representation"
(https://github.com/bitcoin/bitcoin/pull/30214#pullrequestreview-2177291599)
Approach ACK

I think pretty much everything here is a worth while improvement and should set up the following improvements well. I am not so deep into the current state of the kernel work though, so I can not comment on that part, for example the newly introduced typing file. I guess @TheCharlatan could give his nod to that here.

> It removes uses of the terms "active chainstate," "usable chainstate," "disabled chainstate," "ibd chainstate," and "snapshot chainstate" which are confusing fo
...
💬 fjahr commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1677640152)
Not sure why we need this when in another commit we remove `GetAll()` and instead use `m_chainstates`. Seems kind of inconsistent but maybe I am missing something.
💬 fjahr commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1677599944)
Why do you add this function around here instead of using `m_chainstates` as well?
💬 fjahr commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1677573399)
nit: In other places similar checks are inlined without a helper method. That might be actually a bit more clear as well.
👍 hebasto approved a pull request: "OptionsDialog: Prefer to stretch actual options area rather than waste space"
(https://github.com/bitcoin-core/gui/pull/827#pullrequestreview-2177434930)
ACK b71bfd9eef8b0017cef3c05361c02ddbd837e6ac
💬 stickies-v commented on pull request "fuzz: bound some miniscript operations to avoid fuzz timeouts":
(https://github.com/bitcoin/bitcoin/pull/30197#discussion_r1677661313)
Thanks, main concerns are addressed in force push. I'm fine with the current code, but is there a problem I'm not seeing with having `max_subs` be a `size_t`, too? No need to retouch, I'm just curious.
👍 stickies-v approved a pull request: "fuzz: bound some miniscript operations to avoid fuzz timeouts"
(https://github.com/bitcoin/bitcoin/pull/30197#pullrequestreview-2177436401)
Light ACK bc34bc288824978ef4b98e8802b47cb863c8a8c2

Code LGTM and approach seems reasonable, but there may be fuzzing and miniscript nuances I'm not considering.
🚀 hebasto merged a pull request: "OptionsDialog: Prefer to stretch actual options area rather than waste space"
(https://github.com/bitcoin-core/gui/pull/827)
💬 hebasto commented on pull request "depends: remove Darwin ENV unsetting":
(https://github.com/bitcoin/bitcoin/pull/30451#issuecomment-2228264130)
Concept ACK.
💬 fjahr commented on pull request "index: Check all necessary block data is available before starting to sync":
(https://github.com/bitcoin/bitcoin/pull/29770#discussion_r1677672363)
yepp, fixed, thanks!
💬 hebasto commented on pull request "wallet: Improve error log color in the console":
(https://github.com/bitcoin-core/gui/pull/823#issuecomment-2228299766)
cc @GBKS for a designer's opinion.

> I still think the softer color is better.

I'm not a designer, but I find the current implementation the most appealing.

As for implementation, a named constant should be used in the code.
💬 Sjors commented on pull request "Stratum v2 connman":
(https://github.com/bitcoin/bitcoin/pull/30332#issuecomment-2228301130)
Removed the accidentally included `src/common/sv2_messages.cpp`.
💬 maflcko commented on pull request "wallet: Add scan_utxo option to getbalances RPC":
(https://github.com/bitcoin/bitcoin/pull/28930#issuecomment-2228318061)
Are you still working on this? Seems stale for more than half a year, at least.
💬 Sjors commented on pull request "Have createNewBlock() return a BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/30440#issuecomment-2228365527)
Also added `getCoinbaseTx()` and `getWitnessCommitmentIndex()` which are needed for the `NewTemplate` message.

Since this PR now contains Stratum v2 specific stuff anyway, I pulled in #30441. Still in the process of incorporating all this into #29432 to make sure I didn't miss anything.
💬 Sjors commented on pull request "[WIP] Add getCoinbaseMerklePath() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30441#issuecomment-2228366141)
Folded into #30440.