💬 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
...
💬 ryanofsky commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#issuecomment-2240599386)
re: https://github.com/bitcoin/bitcoin/pull/16545#issuecomment-2232487097
> Are you still working on this, or do you want someone to pick it up?
I would like code review, and I probably need to respond to some old comments and questions. There are two parts to this PR:
- The first part of this PR, implemented in the first commit, uses the ALLOW_BOOL, ALLOW_INT, and ALLOW_STRING flags to validate command line and config file settings, and save them into UniValue values as typed values, r
...
(https://github.com/bitcoin/bitcoin/pull/16545#issuecomment-2240599386)
re: https://github.com/bitcoin/bitcoin/pull/16545#issuecomment-2232487097
> Are you still working on this, or do you want someone to pick it up?
I would like code review, and I probably need to respond to some old comments and questions. There are two parts to this PR:
- The first part of this PR, implemented in the first commit, uses the ALLOW_BOOL, ALLOW_INT, and ALLOW_STRING flags to validate command line and config file settings, and save them into UniValue values as typed values, r
...
💬 hebasto commented on pull request "build: Introduce CMake-based buid system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1685163279)
This diff will gone after https://github.com/hebasto/bitcoin/pull/272.
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1685163279)
This diff will gone after https://github.com/hebasto/bitcoin/pull/272.
💬 hebasto commented on issue "bump boost test to 1.59?":
(https://github.com/bitcoin/bitcoin/issues/19128#issuecomment-2240787323)
> To skip tests.
See https://github.com/hebasto/bitcoin/pull/272.
(https://github.com/bitcoin/bitcoin/issues/19128#issuecomment-2240787323)
> To skip tests.
See https://github.com/hebasto/bitcoin/pull/272.
📝 FisZika opened a pull request: "Update translation_process.md"
(https://github.com/bitcoin/bitcoin/pull/30492)
B5690EEEBB952194
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or n
...
(https://github.com/bitcoin/bitcoin/pull/30492)
B5690EEEBB952194
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or n
...
✅ achow101 closed a pull request: "Update translation_process.md"
(https://github.com/bitcoin/bitcoin/pull/30492)
(https://github.com/bitcoin/bitcoin/pull/30492)
📝 achow101 locked a pull request: "Update translation_process.md"
(https://github.com/bitcoin/bitcoin/pull/30492)
B5690EEEBB952194
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or n
...
(https://github.com/bitcoin/bitcoin/pull/30492)
B5690EEEBB952194
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or n
...