💬 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.
💬 TheBlueMatt commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2213907464)
> This seems to be the main argument and I don't find this to be true at all. All lightning implementations, ecash mints, electrum servers, alternative GUIs and block explorers are sidecars. Sure, putting all of these into Bitcoin Core would be convenient for users but it would be an absolute nightmare for Bitcoin Core as a software project. I think this just boils down to how useful the sidecar actually is: if there is demand for the sidecar then users will run it.
And compared to Bitcoin Core
...
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2213907464)
> This seems to be the main argument and I don't find this to be true at all. All lightning implementations, ecash mints, electrum servers, alternative GUIs and block explorers are sidecars. Sure, putting all of these into Bitcoin Core would be convenient for users but it would be an absolute nightmare for Bitcoin Core as a software project. I think this just boils down to how useful the sidecar actually is: if there is demand for the sidecar then users will run it.
And compared to Bitcoin Core
...
💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1668527084)
```
msg_pos=0 sizeof(rt_msghdr)=92 buf.size()=10948
```
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1668527084)
```
msg_pos=0 sizeof(rt_msghdr)=92 buf.size()=10948
```
💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1668531523)
For IPv4:
```
sa->sa_len=16 sa_pos=92 next_msg_pos=144
sa->sa_len=16 sa_pos=108 next_msg_pos=144
sa->sa_len=0 sa_pos=124 next_msg_pos=144
2024-07-08T12:24:02Z [mapport] [net] portmap: Could not determine IPv4 default gateway
```
For IPv6:
```
sa->sa_len=28 sa_pos=92 next_msg_pos=180
sa->sa_len=28 sa_pos=120 next_msg_pos=180
sa->sa_len=0 sa_pos=148 next_msg_pos=180
2024-07-08T12:24:02Z [mapport] [net] portmap: Could not determine IPv6 default gateway
```
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1668531523)
For IPv4:
```
sa->sa_len=16 sa_pos=92 next_msg_pos=144
sa->sa_len=16 sa_pos=108 next_msg_pos=144
sa->sa_len=0 sa_pos=124 next_msg_pos=144
2024-07-08T12:24:02Z [mapport] [net] portmap: Could not determine IPv4 default gateway
```
For IPv6:
```
sa->sa_len=28 sa_pos=92 next_msg_pos=180
sa->sa_len=28 sa_pos=120 next_msg_pos=180
sa->sa_len=0 sa_pos=148 next_msg_pos=180
2024-07-08T12:24:02Z [mapport] [net] portmap: Could not determine IPv6 default gateway
```
👍 alfonsoromanz approved a pull request: "test, assumeutxo: Remove resolved todo comments and add new test"
(https://github.com/bitcoin/bitcoin/pull/30403#pullrequestreview-2163153435)
Re ACK 29d19c414c9e138c99f8de84e398528db92ff07e. The test execution is successful.
I agree that this PR, along with [#30320](https://github.com/bitcoin/bitcoin/pull/30320) and [#29996](https://github.com/bitcoin/bitcoin/pull/29996), would resolve the TODO list completely.
Thanks!
(https://github.com/bitcoin/bitcoin/pull/30403#pullrequestreview-2163153435)
Re ACK 29d19c414c9e138c99f8de84e398528db92ff07e. The test execution is successful.
I agree that this PR, along with [#30320](https://github.com/bitcoin/bitcoin/pull/30320) and [#29996](https://github.com/bitcoin/bitcoin/pull/29996), would resolve the TODO list completely.
Thanks!
💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1668537577)
Maybe replacing `return std::nullopt;` with `continue;`?
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1668537577)
Maybe replacing `return std::nullopt;` with `continue;`?
💬 fjahr commented on pull request "contrib: asmap-tool - Compare ASMaps with respect to specific addresses":
(https://github.com/bitcoin/bitcoin/pull/30246#discussion_r1668537763)
It's fine to leave it as is. It was just a nit :)
(https://github.com/bitcoin/bitcoin/pull/30246#discussion_r1668537763)
It's fine to leave it as is. It was just a nit :)
💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1668563657)
Or maybe the following:
```cpp
if (rt->rtm_addrs & (1 << i)) {
// 2 is just sa_len + sa_family, the theoretical minimum size of a socket address.
if ((sa_pos + 2) > next_msg_pos) return std::nullopt;
const struct sockaddr* sa = (const struct sockaddr*)(buf.data() + sa_pos);
fprintf(stderr, "i=%d sa->sa_len=%hhu sa_pos=%lu next_msg_pos=%zu\n", i, sa->sa_len, sa_pos, next_msg_pos);
if (sa->sa_len >
...
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1668563657)
Or maybe the following:
```cpp
if (rt->rtm_addrs & (1 << i)) {
// 2 is just sa_len + sa_family, the theoretical minimum size of a socket address.
if ((sa_pos + 2) > next_msg_pos) return std::nullopt;
const struct sockaddr* sa = (const struct sockaddr*)(buf.data() + sa_pos);
fprintf(stderr, "i=%d sa->sa_len=%hhu sa_pos=%lu next_msg_pos=%zu\n", i, sa->sa_len, sa_pos, next_msg_pos);
if (sa->sa_len >
...