💬 Sjors commented on pull request "Add Signet launch shortcut for Windows":
(https://github.com/bitcoin/bitcoin/pull/26334#issuecomment-1941508665)
I would also expect not-super-tech-savy people to be testing applications on custom signets, e.g. testing a vault GUI on Inquisition. I would hope they don't use Windows of course :-)

💬 starius commented on pull request "Extend signetchallenge to set target block spacing":
(https://github.com/bitcoin/bitcoin/pull/29365#discussion_r1487877269)
The format of params is extendable in case more fields are added in the future. It is encoded as a concatenation of (field_id, value) tuples, protobuf style. Currently there is only one field defined `pow_target_spacing`, whose field_id is 0x01 and the lendth of encoding is 8 (int64_t). So valid values of params are:

* empty string (checked in `if` block above)
* 0x01 followed by 8 bytes of pow_target_spacing (9 bytes in total)

If length is not 0 and not 9, the value can not be parsed
...

💬 Sjors commented on pull request "Double -dbache maximum to 32GB":
(https://github.com/bitcoin/bitcoin/pull/28358#discussion_r1487922236)
Added a test.

💬 Sjors commented on pull request "Double -dbache maximum to 32GB":
(https://github.com/bitcoin/bitcoin/pull/28358#discussion_r1487922283)
Taken

💬 Sjors commented on pull request "Double -dbache maximum to 32GB":
(https://github.com/bitcoin/bitcoin/pull/28358#discussion_r1487922850)
Taken the suggested text diff.

💬 Sjors commented on pull request "Double -dbache maximum to 32GB":
(https://github.com/bitcoin/bitcoin/pull/28358#discussion_r1487922960)
Updated.

💬 Sjors commented on pull request "Double -dbache maximum to 32GB":
(https://github.com/bitcoin/bitcoin/pull/28358#discussion_r1487923094)
I'm leaving this error as is for now, unless there are other examples of breaking software.

I would also rather drop `nMaxDbCache` entirely than quietly enforce it.

💬 Sjors commented on pull request "Double -dbache maximum to 32GB":
(https://github.com/bitcoin/bitcoin/pull/28358#discussion_r1487924416)
Done

💬 Sjors commented on pull request "Double -dbache maximum to 32GB":
(https://github.com/bitcoin/bitcoin/pull/28358#issuecomment-1941603789)
Rebased, incorporated some of the text feedback above. Dropped the RAM warning from the release note, since that's already in the help text.

> Why do we have an upper limit at all?

@sipa seemed worried about users going overboard with this setting: https://github.com/bitcoin/bitcoin/issues/28249#issuecomment-1695572615

💬 Sjors commented on pull request "Double -dbache maximum to 32GB":
(https://github.com/bitcoin/bitcoin/pull/28358#discussion_r1487928139)
I dropped the "check RAM" sentence.

💬 Sjors commented on pull request "[do not merge] validation: assumeutxo params mainnet":
(https://github.com/bitcoin/bitcoin/pull/28553#issuecomment-1941615713)
I plan to make a fresh snapshot after #26045 lands.

💬 maflcko commented on pull request "fuzz: increase length of string used for `NetWhitelist{bind}Permissions::TryParse`":
(https://github.com/bitcoin/bitcoin/pull/29413#issuecomment-1941644439)
rfm?

💬 maflcko commented on pull request "log: Nuke error(...)":
(https://github.com/bitcoin/bitcoin/pull/29236#discussion_r1487987333)
Yeah, but "error" is still applicable *within* the addrdb functions, as they can not continue and must return. So I think, ideally they'd return `Result<void>`, and the caller could downgrade the error to a warning and log it? Happy to review such a change, if someone creates it.

💬 maflcko commented on pull request "log: Nuke error(...)":
(https://github.com/bitcoin/bitcoin/pull/29236#discussion_r1487995172)
Why? This line is a failure to connect to a peer and the above are proxy errors. So keeping this as-is makes most sense. In any case, if there is a reason to change something unrelated in other lines, a separate pull request would be ideal.

🚀 fanquake merged a pull request: "fuzz: increase length of string used for `NetWhitelist{bind}Permissions::TryParse`"
(https://github.com/bitcoin/bitcoin/pull/29413)

💬 maflcko commented on pull request "log: Nuke error(...)":
(https://github.com/bitcoin/bitcoin/pull/29236#discussion_r1488015627)
If there are changes to other log lines, I'd say it would be best to do them in a separate pull request. The goal here is to be as close to a refactor as possible.

💬 maflcko commented on pull request "Move log messages: tx enqueue to mempool, allocation to blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27277#discussion_r1488036409)
e67ccf09ff3df3810e08d278739fa25335785767: How is this a refactor? You are changing the behavior from debug to trace, no?

Also, I am not sure about this change. This makes intermittent test failures harder to debug, since trace is disabled. Also, below Debug is still used, so this drops only one of two log lines.

💬 glozow commented on pull request "policy: enable sibling eviction for v3 transactions":
(https://github.com/bitcoin/bitcoin/pull/29306#issuecomment-1941768684)
Currently traveling but will push a rebase soon.

El El sáb, 10 feb 2024 a las 1:43, DrahtBot ***@***.***>
escribió:

> 🐙 This pull request conflicts with the target branch and needs rebase
> <https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes>
> .
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/bitcoin/bitcoin/pull/29306#issuecomment-1936861861>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AGAEGGO
...

💬 hebasto commented on issue "RFC: Migration from NSUserNotificationCenter to UNUserNotificationCenter on macOS":
(https://github.com/bitcoin-core/gui/issues/112#issuecomment-1941924302)
After a discussion in https://github.com/bitcoin/bitcoin/pull/29362, this issue has come to our attention again.

In response to concerns raised in https://github.com/bitcoin-core/gui/pull/114, it appears that migrating to the new `UNUserNotificationCenter` remains the only viable option. Therefore, I conducted further research and discovered the following:

The `UNUserNotificationCenter` requires two conditions to be met in order to be granted permissions from the OS:
1. The software must
...

💬 hebasto commented on pull request "build: Add missed definition for `AM_OBJCXXFLAGS`":
(https://github.com/bitcoin/bitcoin/pull/29362#issuecomment-1941936259)
@theuni
> This warning seems legitimate. Looks to me like we should modernize this code rather than ignoring it, no?

I agree with you. More details regarding our options to "modernize this code" are posted in the dedicated https://github.com/bitcoin-core/gui/issues/112.

However, the goal of this PR is to fix the build system to prevent porting this bug into the new CMake-based build system. For the context, please refer to https://github.com/hebasto/bitcoin/pull/84#discussion_r1469371615
...