💬 fanquake commented on issue "test: 32-bit Clang `ipc_test` failure at `-O0`":
(https://github.com/bitcoin/bitcoin/issues/31772#issuecomment-2639590386)
> Other than that, printing a warning when mixing two compilers when compiling with depends may be useful?
Why only depends though? If the issue is generally mixing compilers/flags, then using Clang + system libs on any (gcc-based) Linux system (potentially) has the same problem. It's currently the case that we use Clang + (GCC built) system libs in at least the ASAN,fuzz,valgrind CIs, and Clang + GCC depends libs in the 32-bit job.
(https://github.com/bitcoin/bitcoin/issues/31772#issuecomment-2639590386)
> Other than that, printing a warning when mixing two compilers when compiling with depends may be useful?
Why only depends though? If the issue is generally mixing compilers/flags, then using Clang + system libs on any (gcc-based) Linux system (potentially) has the same problem. It's currently the case that we use Clang + (GCC built) system libs in at least the ASAN,fuzz,valgrind CIs, and Clang + GCC depends libs in the 32-bit job.
💬 jsarenik commented on issue "Fee Estimation via Fee rate Forecasters tracking issue":
(https://github.com/bitcoin/bitcoin/issues/30392#issuecomment-2639597574)
Just few related notes, a compilation from what I have seen so far. HTH, may be biased and YMMV:
Yes, getting a transaction into a block (confirmed) is a future event so on all my nodes I do `ln -nsf /dev/null ~/.bitcoin/fee_estimates.dat` and never call the RPCs guessing "smart fee estimation" from historical values.
Setting a rational initial fee allows for simple (if not m-of-n multisig where `n>=m>1`) RBF replacements to increment the fee just by subtracting the `vsize` sats from one (or m
...
(https://github.com/bitcoin/bitcoin/issues/30392#issuecomment-2639597574)
Just few related notes, a compilation from what I have seen so far. HTH, may be biased and YMMV:
Yes, getting a transaction into a block (confirmed) is a future event so on all my nodes I do `ln -nsf /dev/null ~/.bitcoin/fee_estimates.dat` and never call the RPCs guessing "smart fee estimation" from historical values.
Setting a rational initial fee allows for simple (if not m-of-n multisig where `n>=m>1`) RBF replacements to increment the fee just by subtracting the `vsize` sats from one (or m
...
💬 hebasto commented on pull request "kernel: Avoid duplicating symbols in the kernel and downstream users":
(https://github.com/bitcoin/bitcoin/pull/31807#issuecomment-2639600305)
> tl;dr: Certain symbols when defined in headers may end up existing in both the shared lib and the application using it, leading to subtle bugs. More info can be found [here](https://www.mail-archive.com/cfe-commits@lists.llvm.org/msg503669.html).
>
> The solution is generally to avoid defining static/inline vars in headers if they aren't meant to be shared with downstream kernel users.
I don't think these concerns are relevant to shared kernel library users, as https://github.com/llvm/ll
...
(https://github.com/bitcoin/bitcoin/pull/31807#issuecomment-2639600305)
> tl;dr: Certain symbols when defined in headers may end up existing in both the shared lib and the application using it, leading to subtle bugs. More info can be found [here](https://www.mail-archive.com/cfe-commits@lists.llvm.org/msg503669.html).
>
> The solution is generally to avoid defining static/inline vars in headers if they aren't meant to be shared with downstream kernel users.
I don't think these concerns are relevant to shared kernel library users, as https://github.com/llvm/ll
...
💬 maflcko commented on pull request "rpc, logging: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31177#issuecomment-2639659890)
Just a simple one-line patch to offset `GetBlockTime` with a constant number of hours on top of the code in current master. I don't think there needs to be any special casing?
(https://github.com/bitcoin/bitcoin/pull/31177#issuecomment-2639659890)
Just a simple one-line patch to offset `GetBlockTime` with a constant number of hours on top of the code in current master. I don't think there needs to be any special casing?
💬 hebasto commented on pull request "kernel: Avoid duplicating symbols in the kernel and downstream users":
(https://github.com/bitcoin/bitcoin/pull/31807#issuecomment-2639768171)
> But still, with a c++17-style `inline` variable in a header, it's not clear to me that all linkers are guaranteed to do the right thing in all cases (I admit I might be totally wrong on that), and I would at least like some way to be warned about these so that we can investigate.
I do believe that this case is covered by the [ODR](https://en.cppreference.com/w/cpp/language/definition):
> There can be more than one definition in a program of each of the following: ... inline variable ...
>
...
(https://github.com/bitcoin/bitcoin/pull/31807#issuecomment-2639768171)
> But still, with a c++17-style `inline` variable in a header, it's not clear to me that all linkers are guaranteed to do the right thing in all cases (I admit I might be totally wrong on that), and I would at least like some way to be warned about these so that we can investigate.
I do believe that this case is covered by the [ODR](https://en.cppreference.com/w/cpp/language/definition):
> There can be more than one definition in a program of each of the following: ... inline variable ...
>
...
💬 Sjors commented on pull request "Double check all block rules in `ConnectBlock`, not only `CheckBlock`":
(https://github.com/bitcoin/bitcoin/pull/31792#discussion_r1944480843)
ee05eb440310a1b9d6def8a26442433dffd1a122: this seems nonsensical to me, but it's pre-existing behavior. If you run `-reindex(-chainstate)` on a node with a broken clock, it seems perfectly fine to me that it's going to stop validation with an error. The user can then just fix their clock and continue.
Additionally, what if the users clock was too far in the future and they accidentally accepted a block they shouldn't have. It seems fine that during a reindex, after the user fixed their clock,
...
(https://github.com/bitcoin/bitcoin/pull/31792#discussion_r1944480843)
ee05eb440310a1b9d6def8a26442433dffd1a122: this seems nonsensical to me, but it's pre-existing behavior. If you run `-reindex(-chainstate)` on a node with a broken clock, it seems perfectly fine to me that it's going to stop validation with an error. The user can then just fix their clock and continue.
Additionally, what if the users clock was too far in the future and they accidentally accepted a block they shouldn't have. It seems fine that during a reindex, after the user fixed their clock,
...
💬 Sjors commented on pull request "Double check all block rules in `ConnectBlock`, not only `CheckBlock`":
(https://github.com/bitcoin/bitcoin/pull/31792#discussion_r1944473646)
ee05eb440310a1b9d6def8a26442433dffd1a122: dropping the "Fully solving" part of this remark seems too optimistic. When you upgrade a node after a soft-fork activates, we don't roll back and recheck.
The only thing this commit fixes are `-reindex(-chainstate)` and (manually called) `verifychain` scenarios.
(https://github.com/bitcoin/bitcoin/pull/31792#discussion_r1944473646)
ee05eb440310a1b9d6def8a26442433dffd1a122: dropping the "Fully solving" part of this remark seems too optimistic. When you upgrade a node after a soft-fork activates, we don't roll back and recheck.
The only thing this commit fixes are `-reindex(-chainstate)` and (manually called) `verifychain` scenarios.
💬 Sjors commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2639794378)
@ryanofsky if you need to retouch can you also rebase? That makes it easier for my followup PRs since `00_setup_env_i686_multiprocess.sh` changed in master.
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2639794378)
@ryanofsky if you need to retouch can you also rebase? That makes it easier for my followup PRs since `00_setup_env_i686_multiprocess.sh` changed in master.
💬 Sjors commented on pull request "Double check all block rules in `ConnectBlock`, not only `CheckBlock`":
(https://github.com/bitcoin/bitcoin/pull/31792#discussion_r1944710838)
cc @sdaftuar who wrote this comment in #11737
(https://github.com/bitcoin/bitcoin/pull/31792#discussion_r1944710838)
cc @sdaftuar who wrote this comment in #11737
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1944720234)
Changed. This is now `SockMan::Id` and in `net.h`: `using NodeId = SockMan::Id;`. In SockMan comments change the word "node" to "connection". This caused a lot of mechanical renames within this PR, but it looks better now, thanks!
`SockMan::GetNewNodeId()` renamed to
`SockMan::GetNewId()`
`SockMan::EventIOLoopCompletedForNode()` renamed to
`SockMan::EventIOLoopCompletedForOne()`
`SockMan::EventIOLoopCompletedForAllPeers()` renamed to
`SockMan::EventIOLoopCompletedForAll()`
`SockMa
...
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1944720234)
Changed. This is now `SockMan::Id` and in `net.h`: `using NodeId = SockMan::Id;`. In SockMan comments change the word "node" to "connection". This caused a lot of mechanical renames within this PR, but it looks better now, thanks!
`SockMan::GetNewNodeId()` renamed to
`SockMan::GetNewId()`
`SockMan::EventIOLoopCompletedForNode()` renamed to
`SockMan::EventIOLoopCompletedForOne()`
`SockMan::EventIOLoopCompletedForAllPeers()` renamed to
`SockMan::EventIOLoopCompletedForAll()`
`SockMa
...
💬 Sjors commented on pull request "Double check all block rules in `ConnectBlock`, not only `CheckBlock`":
(https://github.com/bitcoin/bitcoin/pull/31792#issuecomment-2639841215)
Some historical context. The 2 hour future check used to be in `CheckBlockHeader`, but was moved in #8068 (Compact Blocks) in order to "prevent yet more includes and the crazy-looking GetAdjustedTime call from within the GetBlock stuff.": https://github.com/bitcoin/bitcoin/pull/8068/files#r66835690
> In order to avoid checking the merkle tree twice (which is incredibly expensive), I had to call CheckBlock, which required three #includes (including main.h) to blockencodings.cpp, despite them b
...
(https://github.com/bitcoin/bitcoin/pull/31792#issuecomment-2639841215)
Some historical context. The 2 hour future check used to be in `CheckBlockHeader`, but was moved in #8068 (Compact Blocks) in order to "prevent yet more includes and the crazy-looking GetAdjustedTime call from within the GetBlock stuff.": https://github.com/bitcoin/bitcoin/pull/8068/files#r66835690
> In order to avoid checking the merkle tree twice (which is incredibly expensive), I had to call CheckBlock, which required three #includes (including main.h) to blockencodings.cpp, despite them b
...
💬 ryanofsky commented on issue "test: 32-bit Clang `ipc_test` failure at `-O0`":
(https://github.com/bitcoin/bitcoin/issues/31772#issuecomment-2639854388)
> Why only depends though? If the issue is generally mixing compilers/flags, then using Clang + system libs on any (gcc-based) Linux system (potentially) has the same problem.
Yes this issue is not specific to depends, and could theoretically could happen if, for example an ubuntu library package was compiled with gcc and exposed a function with an empty struct parameter, and you tried to use the library with clang.
The point of having an ABI is to prevent issues like this and allow different
...
(https://github.com/bitcoin/bitcoin/issues/31772#issuecomment-2639854388)
> Why only depends though? If the issue is generally mixing compilers/flags, then using Clang + system libs on any (gcc-based) Linux system (potentially) has the same problem.
Yes this issue is not specific to depends, and could theoretically could happen if, for example an ubuntu library package was compiled with gcc and exposed a function with an empty struct parameter, and you tried to use the library with clang.
The point of having an ABI is to prevent issues like this and allow different
...
💬 arejula27 commented on pull request "test: expect that files may disappear from /proc/PID/fd/":
(https://github.com/bitcoin/bitcoin/pull/31614#issuecomment-2639920298)
ACK [`b2e9fdc`](https://github.com/bitcoin/bitcoin/pull/31614/commits/b2e9fdc00f5c40c241a37739f7b73b74c2181e39)
The issue is that the process of getting the list of files in the directory and then reading each one is not atomic. This can lead to race conditions. The proposed solution doesn’t fully fix the problem but avoids its consequences, preventing the test from failing when it shouldn’t.
Following @theuni’s idea, I tried exploring other solutions using `scandir`:
```pyhton
for item
...
(https://github.com/bitcoin/bitcoin/pull/31614#issuecomment-2639920298)
ACK [`b2e9fdc`](https://github.com/bitcoin/bitcoin/pull/31614/commits/b2e9fdc00f5c40c241a37739f7b73b74c2181e39)
The issue is that the process of getting the list of files in the directory and then reading each one is not atomic. This can lead to race conditions. The proposed solution doesn’t fully fix the problem but avoids its consequences, preventing the test from failing when it shouldn’t.
Following @theuni’s idea, I tried exploring other solutions using `scandir`:
```pyhton
for item
...
💬 maflcko commented on issue "Continuous benchmark tracking":
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-2639922568)
corecheck is back up and integrated into the DrahtBot comment, so is anything left to be done here?
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-2639922568)
corecheck is back up and integrated into the DrahtBot comment, so is anything left to be done here?
💬 ryanofsky commented on issue "test: 32-bit Clang `ipc_test` failure at `-O0`":
(https://github.com/bitcoin/bitcoin/issues/31772#issuecomment-2639932130)
Submitted patch in https://github.com/capnproto/capnproto/pull/2235
(https://github.com/bitcoin/bitcoin/issues/31772#issuecomment-2639932130)
Submitted patch in https://github.com/capnproto/capnproto/pull/2235
💬 kevkevinpal commented on pull request "doc: improve NODE_NETWORK_LIMITED documentation per BIP159":
(https://github.com/bitcoin/bitcoin/pull/31805#issuecomment-2639939670)
ACK [39e15f6](https://github.com/bitcoin/bitcoin/pull/31805/commits/39e15f6ca4fd44cc459d80b7a1540c03923906dd)
I agree with @maflcko maybe instead of using pruned node we can refer to the node as "network limited node"
(https://github.com/bitcoin/bitcoin/pull/31805#issuecomment-2639939670)
ACK [39e15f6](https://github.com/bitcoin/bitcoin/pull/31805/commits/39e15f6ca4fd44cc459d80b7a1540c03923906dd)
I agree with @maflcko maybe instead of using pruned node we can refer to the node as "network limited node"
📝 glozow opened a pull request: "TxOrphanage: account for size of orphans and count announcements"
(https://github.com/bitcoin/bitcoin/pull/31810)
Part of orphan resolution project, see #27463.
Definitions:
- **Announcement** is a unique pair (wtxid, nodeid). We can have multiple announcers for the same orphan since #31397.
- **Size** is the weight of an orphan. I'm calling it "size" and "bytes" because I think we can refine it in the future to be memusage or be otherwise more representative of the orphan's actual cost on our memory. However, I am open to naming changes.
This is part 1/2 of a project to also add limits on orphan si
...
(https://github.com/bitcoin/bitcoin/pull/31810)
Part of orphan resolution project, see #27463.
Definitions:
- **Announcement** is a unique pair (wtxid, nodeid). We can have multiple announcers for the same orphan since #31397.
- **Size** is the weight of an orphan. I'm calling it "size" and "bytes" because I think we can refine it in the future to be memusage or be otherwise more representative of the orphan's actual cost on our memory. However, I am open to naming changes.
This is part 1/2 of a project to also add limits on orphan si
...
💬 instagibbs commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#issuecomment-2639947796)
I did some *very* light fuzzing against `CFeeRate` results to make sure the "gap" between the two values is never huge, would there be some value in adding coverage to confirm that results aren't wildly off from what we have today if we use it more widely?
(https://github.com/bitcoin/bitcoin/pull/30535#issuecomment-2639947796)
I did some *very* light fuzzing against `CFeeRate` results to make sure the "gap" between the two values is never huge, would there be some value in adding coverage to confirm that results aren't wildly off from what we have today if we use it more widely?
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1944816545)
Changed to `EventI2PStatus()` and instead of `bool` it now takes an enum with (currently) two possible values: `START_LISTENING` and `STOP_LISTENING`. Will be in next push.
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1944816545)
Changed to `EventI2PStatus()` and instead of `bool` it now takes an enum with (currently) two possible values: `START_LISTENING` and `STOP_LISTENING`. Will be in next push.
💬 sipa commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#issuecomment-2639972637)
> would there be some value in adding coverage to confirm that results aren't wildly off from what we have today if we use it more widely?
Sure, happy to add a commit for tests with that.
(https://github.com/bitcoin/bitcoin/pull/30535#issuecomment-2639972637)
> would there be some value in adding coverage to confirm that results aren't wildly off from what we have today if we use it more widely?
Sure, happy to add a commit for tests with that.