✅ Sjors closed a pull request: "Stratum v2 connman"
(https://github.com/bitcoin/bitcoin/pull/30332)
(https://github.com/bitcoin/bitcoin/pull/30332)
💬 Sjors commented on pull request "Stratum v2 connman":
(https://github.com/bitcoin/bitcoin/pull/30332#issuecomment-2239955808)
I moved this PR to my own fork at https://github.com/Sjors/bitcoin/pull/50 to reduce CI load, which apparently was becoming too much: https://github.com/bitcoin/bitcoin/pull/30475#issuecomment-2238759084
(https://github.com/bitcoin/bitcoin/pull/30332#issuecomment-2239955808)
I moved this PR to my own fork at https://github.com/Sjors/bitcoin/pull/50 to reduce CI load, which apparently was becoming too much: https://github.com/bitcoin/bitcoin/pull/30475#issuecomment-2238759084
✅ Sjors closed a pull request: "Stratum v2 Template Provider common functionality"
(https://github.com/bitcoin/bitcoin/pull/30475)
(https://github.com/bitcoin/bitcoin/pull/30475)
💬 Sjors commented on pull request "Stratum v2 Template Provider common functionality":
(https://github.com/bitcoin/bitcoin/pull/30475#issuecomment-2239959662)
Managed to fix the CI issue.
Moving this PR to https://github.com/Sjors/bitcoin/pull/49 to reduce the PR stack here a bit and not overwhelm CI.
(https://github.com/bitcoin/bitcoin/pull/30475#issuecomment-2239959662)
Managed to fix the CI issue.
Moving this PR to https://github.com/Sjors/bitcoin/pull/49 to reduce the PR stack here a bit and not overwhelm CI.
👍 hebasto approved a pull request: "depends: Fix CMake-generated `libevent*.pc` files"
(https://github.com/bitcoin/bitcoin/pull/30488#pullrequestreview-2189047606)
ACK 8c935e625ea75d180144f0526d6a0d5fd58c1f29.
(https://github.com/bitcoin/bitcoin/pull/30488#pullrequestreview-2189047606)
ACK 8c935e625ea75d180144f0526d6a0d5fd58c1f29.
👍 ryanofsky approved a pull request: "depends: bump libmultiprocess for CMake fixes"
(https://github.com/bitcoin/bitcoin/pull/30490#pullrequestreview-2189098956)
Code review ACK d318c4ef56465ccad1a1d4d27c52216e0b69ad4e.
I haven't tested the cmake support, but I made the same version bump in 3e6c61fdc2839bdb74563538aaf0a5e7d0e07ea3, which is part of #10102 and 30437
(https://github.com/bitcoin/bitcoin/pull/30490#pullrequestreview-2189098956)
Code review ACK d318c4ef56465ccad1a1d4d27c52216e0b69ad4e.
I haven't tested the cmake support, but I made the same version bump in 3e6c61fdc2839bdb74563538aaf0a5e7d0e07ea3, which is part of #10102 and 30437
💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#issuecomment-2240082764)
Thanks for the feedback! Helped me realize I was being too terse. (I think it was a mistake alone just to not have `string_view` in the title for the sake of keeping under 51 chars).
Only modified commit message:
```diff
--- c3a9c71c7077324bf0aa40f834f7537a14619340 2024-07-19 22:42:56.019823786 +0200
+++ 649c88d4df0c97cd0109c0cd0e54bc76f2287ef2 2024-07-19 22:42:57.539661157 +0200
@@ -1,3 +1,5 @@
-fix: Make TxidFromString() respect string length
+fix: Make TxidFromString() respect string
...
(https://github.com/bitcoin/bitcoin/pull/30436#issuecomment-2240082764)
Thanks for the feedback! Helped me realize I was being too terse. (I think it was a mistake alone just to not have `string_view` in the title for the sake of keeping under 51 chars).
Only modified commit message:
```diff
--- c3a9c71c7077324bf0aa40f834f7537a14619340 2024-07-19 22:42:56.019823786 +0200
+++ 649c88d4df0c97cd0109c0cd0e54bc76f2287ef2 2024-07-19 22:42:57.539661157 +0200
@@ -1,3 +1,5 @@
-fix: Make TxidFromString() respect string length
+fix: Make TxidFromString() respect string
...
💬 hodlinator commented on pull request "refactor: Add consteval uint256("str") and ParseHex("str")":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1684964695)
I think it's more acceptable to diverge on behavior if we call the `consteval` function something different, see https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1682782077
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1684964695)
I think it's more acceptable to diverge on behavior if we call the `consteval` function something different, see https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1682782077
📝 hebasto opened a pull request: "Fix MSVC warning C4273 "inconsistent dll linkage""
(https://github.com/bitcoin/bitcoin/pull/30491)
Broken out of https://github.com/bitcoin/bitcoin/pull/30454.
When using CMake, the user can select the MSVC runtime library to be:
1) Statically-linked (with the corresponding `x64-windows-static` vcpkg triplet) or
2) Dynamically-linked (with the corresponding `x64-windows` vcpkg triplet)
In the latter case, the compiler emits the [C4273](https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-1-c4273) warning.
As the "Necessary on some platforms"
...
(https://github.com/bitcoin/bitcoin/pull/30491)
Broken out of https://github.com/bitcoin/bitcoin/pull/30454.
When using CMake, the user can select the MSVC runtime library to be:
1) Statically-linked (with the corresponding `x64-windows-static` vcpkg triplet) or
2) Dynamically-linked (with the corresponding `x64-windows` vcpkg triplet)
In the latter case, the compiler emits the [C4273](https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-1-c4273) warning.
As the "Necessary on some platforms"
...
💬 hebasto commented on pull request "Fix MSVC warning C4273 "inconsistent dll linkage"":
(https://github.com/bitcoin/bitcoin/pull/30491#issuecomment-2240116123)
Friendly ping:
- @theuni per offline request for this PR
- @sipsorcery as a Windows connoisseur
- @sipa as an author of the [original code](https://github.com/bitcoin/bitcoin/commit/2554c1b81bb8c40e1989025c6f18e7935720b156)
(https://github.com/bitcoin/bitcoin/pull/30491#issuecomment-2240116123)
Friendly ping:
- @theuni per offline request for this PR
- @sipsorcery as a Windows connoisseur
- @sipa as an author of the [original code](https://github.com/bitcoin/bitcoin/commit/2554c1b81bb8c40e1989025c6f18e7935720b156)
💬 sipa commented on pull request "Fix MSVC warning C4273 "inconsistent dll linkage"":
(https://github.com/bitcoin/bitcoin/pull/30491#issuecomment-2240124093)
utACK 7703884ab19cd7ffddc5c52ba57dec82fbc8dc2b
(https://github.com/bitcoin/bitcoin/pull/30491#issuecomment-2240124093)
utACK 7703884ab19cd7ffddc5c52ba57dec82fbc8dc2b
💬 sipsorcery commented on pull request "Fix MSVC warning C4273 "inconsistent dll linkage"":
(https://github.com/bitcoin/bitcoin/pull/30491#issuecomment-2240169797)
utACK 7703884ab19cd7ffddc5c52ba57dec82fbc8dc2b.
(https://github.com/bitcoin/bitcoin/pull/30491#issuecomment-2240169797)
utACK 7703884ab19cd7ffddc5c52ba57dec82fbc8dc2b.
💬 hodlinator commented on pull request "refactor: Add consteval uint256("str") and ParseHex("str")":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1685019567)
> Seems fine to change later to string_view, if this is needed.
>
> However, I wonder if you can replace `65` by `WIDTH+1`?
Done now in 79921003ffc858ca4b47e0fb187ed83c1667bc27 along with added comment and more descriptive parameter name.
> Also, I wonder if the three duplicate redirects can be replaced by a single uinsg base_blob::base_blob; (or similar) to import all constructors.
I guess that would only be possible if switching the uint256-ctor to `string_view`, which I'd rather h
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1685019567)
> Seems fine to change later to string_view, if this is needed.
>
> However, I wonder if you can replace `65` by `WIDTH+1`?
Done now in 79921003ffc858ca4b47e0fb187ed83c1667bc27 along with added comment and more descriptive parameter name.
> Also, I wonder if the three duplicate redirects can be replaced by a single uinsg base_blob::base_blob; (or similar) to import all constructors.
I guess that would only be possible if switching the uint256-ctor to `string_view`, which I'd rather h
...
🤔 ryanofsky reviewed a pull request: "refactor: Improve assumeutxo state representation"
(https://github.com/bitcoin/bitcoin/pull/30214#pullrequestreview-2189391816)
Rebased 493d3844cfc8628fd63184fa7a657126d9b5dc95 -> 2c1d8b426fa1832fa464c12f3cae7e2976584430 ([`pr/cstate.4`](https://github.com/ryanofsky/bitcoin/commits/pr/cstate.4) -> [`pr/cstate.5`](https://github.com/ryanofsky/bitcoin/commits/pr/cstate.5), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/cstate.4-rebase..pr/cstate.5)) due to conflicts with various prs including #30267, #30388, #30395, #29996, #30406, #30320. Also made suggested simplifications.
(https://github.com/bitcoin/bitcoin/pull/30214#pullrequestreview-2189391816)
Rebased 493d3844cfc8628fd63184fa7a657126d9b5dc95 -> 2c1d8b426fa1832fa464c12f3cae7e2976584430 ([`pr/cstate.4`](https://github.com/ryanofsky/bitcoin/commits/pr/cstate.4) -> [`pr/cstate.5`](https://github.com/ryanofsky/bitcoin/commits/pr/cstate.5), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/cstate.4-rebase..pr/cstate.5)) due to conflicts with various prs including #30267, #30388, #30395, #29996, #30406, #30320. Also made suggested simplifications.
💬 ryanofsky commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1685023239)
re: 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.
You're right, that is clearer. Updated different places where this was used.
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1685023239)
re: 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.
You're right, that is clearer. Updated different places where this was used.
💬 ryanofsky commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1685023046)
re: https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1677599944
> Why do you add this function around here instead of using `m_chainstates` as well?
Dropped this function now, it was not hard to get rid of.
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1685023046)
re: https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1677599944
> Why do you add this function around here instead of using `m_chainstates` as well?
Dropped this function now, it was not hard to get rid of.
💬 hebasto commented on pull request "build: use `-no_exported_symbols` on macOS":
(https://github.com/bitcoin/bitcoin/pull/29072#issuecomment-2240312745)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/271.
(https://github.com/bitcoin/bitcoin/pull/29072#issuecomment-2240312745)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/271.
👍 theuni approved a pull request: "Fix MSVC warning C4273 "inconsistent dll linkage""
(https://github.com/bitcoin/bitcoin/pull/30491#pullrequestreview-2189430254)
Microsoft says on Windows it's coming from `Stdlib.h`, which something else must be including. (Well, Microsoft says it's called `_environ`, but I'm pretty sure the linker fixes that up for us).
From what I can tell, this will simply fail to link if it's not found _somewhere_. Also, this is just another opportunistic entropy source for us, so setting aside a Debian+OpenSSL-type whoopsie, I don't think it's a huge deal if this is null anyway.
So.. as long as this doesn't break anything, fi
...
(https://github.com/bitcoin/bitcoin/pull/30491#pullrequestreview-2189430254)
Microsoft says on Windows it's coming from `Stdlib.h`, which something else must be including. (Well, Microsoft says it's called `_environ`, but I'm pretty sure the linker fixes that up for us).
From what I can tell, this will simply fail to link if it's not found _somewhere_. Also, this is just another opportunistic entropy source for us, so setting aside a Debian+OpenSSL-type whoopsie, I don't think it's a huge deal if this is null anyway.
So.. as long as this doesn't break anything, fi
...
💬 davidgumberg commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1685047415)
It seems to me the cache entry is not necessarily newly created here, if e.g. cache is not flushed and an entry is `DIRTY` and spent when a reorg takes place.
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1685047415)
It seems to me the cache entry is not necessarily newly created here, if e.g. cache is not flushed and an entry is `DIRTY` and spent when a reorg takes place.
🤔 furszy reviewed a pull request: "index: Check all necessary block data is available before starting to sync"
(https://github.com/bitcoin/bitcoin/pull/29770#pullrequestreview-2189455441)
> But the index still needs to have an understanding of what block data is and what it needs in order to decide if it can handle what the the kernel gives it. So I don't think it can be avoided that the index has knowledge of the block status so this would need to be shared. That doesn't mean that they can't run isolation, but the kernel needs to share this knowledge with them. We can only avoid this if we forget about the concept suggested here and let the index ask for data from the kernel unt
...
(https://github.com/bitcoin/bitcoin/pull/29770#pullrequestreview-2189455441)
> But the index still needs to have an understanding of what block data is and what it needs in order to decide if it can handle what the the kernel gives it. So I don't think it can be avoided that the index has knowledge of the block status so this would need to be shared. That doesn't mean that they can't run isolation, but the kernel needs to share this knowledge with them. We can only avoid this if we forget about the concept suggested here and let the index ask for data from the kernel unt
...