💬 rkrux commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1668268758)
```
// Create cluster D child ---> K
```
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1668268758)
```
// Create cluster D child ---> K
```
💬 rkrux commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1668306440)
Interesting that `tx_set` is passed by value here and 2 separate copies are made at the time of calling, which is the need here for separate ancestors and descendants copies.
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1668306440)
Interesting that `tx_set` is passed by value here and 2 separate copies are made at the time of calling, which is the need here for separate ancestors and descendants copies.
💬 rkrux commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1668295683)
+1 on separating this function out in a file and adding focussed unit tests for it, I feel this is generic enough to be used in several places later.
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1668295683)
+1 on separating this function out in a file and adding focussed unit tests for it, I feel this is generic enough to be used in several places later.
🤔 glozow reviewed a pull request: "kernel: De-globalize validation caches"
(https://github.com/bitcoin/bitcoin/pull/30141#pullrequestreview-2162794247)
reACK 606a7ab
(https://github.com/bitcoin/bitcoin/pull/30141#pullrequestreview-2162794247)
reACK 606a7ab
💬 ismaelsadeeq commented on pull request "BlockAssembler: return selected packages vsize and feerate":
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1668354967)
thanks the commit is gone now.
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1668354967)
thanks the commit is gone now.
🤔 glozow reviewed a pull request: "Use `WITH_LOCK` in `Warnings::Set`"
(https://github.com/bitcoin/bitcoin/pull/30404#pullrequestreview-2162862078)
ACK 6af51e819847e737449609daa214e16f9453e85d
Not familiar with this code at all, but it looks like the handler function `handleNotifyAlertChanged` calls `getStatusBarWarnings`
https://github.com/bitcoin/bitcoin/blob/bd5d1688b4311e21c0e0ff89a3ae02ef7d0543b8/src/qt/clientmodel.cpp#L256-L260
which calls `Node::getWarnings()`
https://github.com/bitcoin/bitcoin/blob/bd5d1688b4311e21c0e0ff89a3ae02ef7d0543b8/src/qt/clientmodel.cpp#L164-L167
which calls `GetMessages()`
https://github.com/bitcoi
...
(https://github.com/bitcoin/bitcoin/pull/30404#pullrequestreview-2162862078)
ACK 6af51e819847e737449609daa214e16f9453e85d
Not familiar with this code at all, but it looks like the handler function `handleNotifyAlertChanged` calls `getStatusBarWarnings`
https://github.com/bitcoin/bitcoin/blob/bd5d1688b4311e21c0e0ff89a3ae02ef7d0543b8/src/qt/clientmodel.cpp#L256-L260
which calls `Node::getWarnings()`
https://github.com/bitcoin/bitcoin/blob/bd5d1688b4311e21c0e0ff89a3ae02ef7d0543b8/src/qt/clientmodel.cpp#L164-L167
which calls `GetMessages()`
https://github.com/bitcoi
...
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1668366154)
this should probably be moved to a [later commit](https://github.com/bitcoin/bitcoin/pull/28280/commits/92aa98712c604b3e424671b196b8425fabdf74d0#diff-095ce1081a930998a10b37358fae5499ac47f8cb6f25f5df5d88e920a54e0341R189) where you introduce the `Next()` method
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1668366154)
this should probably be moved to a [later commit](https://github.com/bitcoin/bitcoin/pull/28280/commits/92aa98712c604b3e424671b196b8425fabdf74d0#diff-095ce1081a930998a10b37358fae5499ac47f8cb6f25f5df5d88e920a54e0341R189) where you introduce the `Next()` method
💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1668367047)
> some lame form of authentication
https://github.com/miniupnp/miniupnp/issues/748 (I still need to test the fix PR)
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1668367047)
> some lame form of authentication
https://github.com/miniupnp/miniupnp/issues/748 (I still need to test the fix PR)
💬 instagibbs commented on pull request "build: Bump clang minimum supported version to 16":
(https://github.com/bitcoin/bitcoin/pull/30263#issuecomment-2213646897)
concept ACK
(https://github.com/bitcoin/bitcoin/pull/30263#issuecomment-2213646897)
concept ACK
💬 dergoegge commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2213654157)
It looks like we agree that in terms of efficiency the sidecar approach is not a problem, so I'll argue the other points.
> One important goal there is to support providing that work to untrusted open connections easily
From my perspective, the sidecar approach allows to solve this in a much nicer way (no additional burden on bitcoin core, sidecar can be written in a memory safe language, ...).
> We've seen time and time again "just run this sidecar software next to Bitcoin Core" is not
...
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2213654157)
It looks like we agree that in terms of efficiency the sidecar approach is not a problem, so I'll argue the other points.
> One important goal there is to support providing that work to untrusted open connections easily
From my perspective, the sidecar approach allows to solve this in a much nicer way (no additional burden on bitcoin core, sidecar can be written in a memory safe language, ...).
> We've seen time and time again "just run this sidecar software next to Bitcoin Core" is not
...
👍 dergoegge approved a pull request: "fuzz: bound some miniscript operations to avoid fuzz timeouts"
(https://github.com/bitcoin/bitcoin/pull/30197#pullrequestreview-2162956899)
utACK 178a1da9c4da955c3dc78e6b03f110f687e82508
(https://github.com/bitcoin/bitcoin/pull/30197#pullrequestreview-2162956899)
utACK 178a1da9c4da955c3dc78e6b03f110f687e82508
💬 stickies-v commented on issue "Double lock detected in `Warnings::GetMessages()`":
(https://github.com/bitcoin/bitcoin/issues/30400#issuecomment-2213692311)
Thanks a lot for finding this @achow101
The bug was introduced in 9c4b0b7ce459765fa1a63b410c3423b90f0d2a5f and affects all `bitcoin-qt` instances on either net. If `bitcoin-qt` was compiled with `--enable-debug` it will lead to a crash, otherwise it will lead to the `scheduler` thread being deadlocked and effectively halting the node. `bitcoind` is not affected by this bug as far as I can see.
Another way to reproduce the bug quickly (on any net) is to:
1. manually shift your system cloc
...
(https://github.com/bitcoin/bitcoin/issues/30400#issuecomment-2213692311)
Thanks a lot for finding this @achow101
The bug was introduced in 9c4b0b7ce459765fa1a63b410c3423b90f0d2a5f and affects all `bitcoin-qt` instances on either net. If `bitcoin-qt` was compiled with `--enable-debug` it will lead to a crash, otherwise it will lead to the `scheduler` thread being deadlocked and effectively halting the node. `bitcoind` is not affected by this bug as far as I can see.
Another way to reproduce the bug quickly (on any net) is to:
1. manually shift your system cloc
...
💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2213708169)
Compared to faacd264417bd7f3f08c5ff497458030b3a54fbc I'm now getting "Could not determine [IPv4/IPv6] default gateway". The changes to `defined(__APPLE__)` in `netif.cpp` don't give me any immediate hint of what could explain this.
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2213708169)
Compared to faacd264417bd7f3f08c5ff497458030b3a54fbc I'm now getting "Could not determine [IPv4/IPv6] default gateway". The changes to `defined(__APPLE__)` in `netif.cpp` don't give me any immediate hint of what could explain this.
💬 dergoegge commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1668437385)
You mean test a global and local cache? We shouldn't fuzz globals (unless we can reset their state).
The reason I commented is that globals make fuzz tests non-deterministic. Let's say the harness is called (in-process) with input A, B and then C. It crashes on input C (which will be the input that the fuzz engine reports to you) but it only crashed because of the data stored in the global cache from input A and B. Giving the harness just input C won't make it crash, because it depends on the
...
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1668437385)
You mean test a global and local cache? We shouldn't fuzz globals (unless we can reset their state).
The reason I commented is that globals make fuzz tests non-deterministic. Let's say the harness is called (in-process) with input A, B and then C. It crashes on input C (which will be the input that the fuzz engine reports to you) but it only crashed because of the data stored in the global cache from input A and B. Giving the harness just input C won't make it crash, because it depends on the
...
💬 maflcko commented on pull request "policy: Add PayToAnchor(P2A), `OP_TRUE <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1668438746)
5e7e767bc79682de3879b8546294d50542570092: Intentional to make p2sh-p2a spend standard as well? Would be good to have a test as documentation, so that reviewers know what to expect and don't have to guess.
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1668438746)
5e7e767bc79682de3879b8546294d50542570092: Intentional to make p2sh-p2a spend standard as well? Would be good to have a test as documentation, so that reviewers know what to expect and don't have to guess.
💬 fjahr commented on pull request "test: Remove already resolved assumeutxo todo comment":
(https://github.com/bitcoin/bitcoin/pull/30403#issuecomment-2213718691)
> I wonder if this is a good place to discuss other (potentially addressed) TODOs?
Right, good idea. I avoided this for a while because I thought I might miss something because I misinterpret them but since we had these detailed discussions in #29996 I am now pretty confident we can remove some that are addressed.
> For example, interesting starting states could be loading a snapshot when the current chain tip is:
>
> `- TODO: An ancestor of snapshot block`: isn't this already addressed
...
(https://github.com/bitcoin/bitcoin/pull/30403#issuecomment-2213718691)
> I wonder if this is a good place to discuss other (potentially addressed) TODOs?
Right, good idea. I avoided this for a while because I thought I might miss something because I misinterpret them but since we had these detailed discussions in #29996 I am now pretty confident we can remove some that are addressed.
> For example, interesting starting states could be loading a snapshot when the current chain tip is:
>
> `- TODO: An ancestor of snapshot block`: isn't this already addressed
...
💬 fjahr commented on pull request "test: Remove already resolved assumeutxo todo comment":
(https://github.com/bitcoin/bitcoin/pull/30403#issuecomment-2213733504)
I have amended the commit to remove the previously named comments + made @alfonsoromanz co-author.
In the second commit I now resolve one of the few remaining cases where the tip is on the same block as the snapshot. This is a special case of the "more work" case so this was quite simple to do.
I think this means with this and the remaining open test PRs from @alfonsoromanz and @mzumsande we could resolve this list completely :)
(https://github.com/bitcoin/bitcoin/pull/30403#issuecomment-2213733504)
I have amended the commit to remove the previously named comments + made @alfonsoromanz co-author.
In the second commit I now resolve one of the few remaining cases where the tip is on the same block as the snapshot. This is a special case of the "more work" case so this was quite simple to do.
I think this means with this and the remaining open test PRs from @alfonsoromanz and @mzumsande we could resolve this list completely :)
💬 fjahr commented on pull request "assumeutxo: Don't load a snapshot if it's not in the best header chain":
(https://github.com/bitcoin/bitcoin/pull/30320#discussion_r1668453002)
Actually also this TODO, I think these basically mean the same thing: `- TODO: Not an ancestor or a descendant of the snapshot block and has more work`
(https://github.com/bitcoin/bitcoin/pull/30320#discussion_r1668453002)
Actually also this TODO, I think these basically mean the same thing: `- TODO: Not an ancestor or a descendant of the snapshot block and has more work`
💬 fjahr commented on pull request "test, assumeutxo: Remove resolved todo comments and add new test":
(https://github.com/bitcoin/bitcoin/pull/30403#issuecomment-2213758446)
To give more detail where they are resolved: I think #30320 resolves two of the remaining three, it's just not part of the PR yet, see: https://github.com/bitcoin/bitcoin/pull/30320#discussion_r1665811272 and #29996 removes the third as already done in the commit here: https://github.com/bitcoin/bitcoin/pull/29996/commits/5b7f70ba2661a194a3c476b81e03785feddb4e1c.
The last one to get merged gets to remove the whole comment section 🥳
(https://github.com/bitcoin/bitcoin/pull/30403#issuecomment-2213758446)
To give more detail where they are resolved: I think #30320 resolves two of the remaining three, it's just not part of the PR yet, see: https://github.com/bitcoin/bitcoin/pull/30320#discussion_r1665811272 and #29996 removes the third as already done in the commit here: https://github.com/bitcoin/bitcoin/pull/29996/commits/5b7f70ba2661a194a3c476b81e03785feddb4e1c.
The last one to get merged gets to remove the whole comment section 🥳
💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1668523385)
9a067b5ac154154ec9008a0e8f2d8f03e994a589: this is triggered on my Intel macOS 14.5 machine. If I comment out the `if` statement it works fine.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1668523385)
9a067b5ac154154ec9008a0e8f2d8f03e994a589: this is triggered on my Intel macOS 14.5 machine. If I comment out the `if` statement it works fine.