💬 theuni commented on pull request "depends: bump libmultiprocess for CMake fixes":
(https://github.com/bitcoin/bitcoin/pull/30490#issuecomment-2239894232)
Ping @ryanofsky for a quick concept ACK for bumping to this particular commit.
(https://github.com/bitcoin/bitcoin/pull/30490#issuecomment-2239894232)
Ping @ryanofsky for a quick concept ACK for bumping to this particular commit.
✅ 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.