💬 sipa commented on pull request "Fix MSVC warning C4273 "inconsistent dll linkage"":
(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.

💬 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
...

🤔 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.

💬 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.

💬 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.

💬 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.

👍 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
...

💬 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.

🤔 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
...

💬 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
...

💬 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.

💬 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.

📝 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
...

✅ achow101 closed a pull request: "Update translation_process.md"
(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
...

📝 1440000bytes opened a pull request: "policy: enable full-rbf by default"
(https://github.com/bitcoin/bitcoin/pull/30493)
This pull request enables full rbf (mempool policy) by default. Commits are same as #28132 however it was closed recently with this [comment](https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-2225369634).

---

Rationale:

- Full RBF config option was added in July 2022: https://github.com/bitcoin/bitcoin/pull/25353

- It is used regularly: https://mempool.space/rbf#fullrbf

- Most mining pools are using it: https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-2059120917
...

💬 hebasto commented on pull request "build: Introduce CMake-based buid system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1685289194)
Thanks! Fixed in https://github.com/hebasto/bitcoin/pull/273.

💬 hebasto commented on pull request "[DO NOT MERGE] cmake: Migrate CI scripts to CMake-based build system -- WIP":
(https://github.com/bitcoin/bitcoin/pull/29790#issuecomment-2240971961)
Temporarily reopening this PR to demonstrate the correctness of the https://github.com/hebasto/bitcoin/pull/273.

📝 hebasto reopened a pull request: "[DO NOT MERGE] cmake: Migrate CI scripts to CMake-based build system -- WIP"
(https://github.com/bitcoin/bitcoin/pull/29790)
This PR goal is to facilitate testing CI scripts migration to the CMake.