💬 TheCharlatan commented on pull request "Extend signetchallenge to set target block spacing":
(https://github.com/bitcoin/bitcoin/pull/29365#issuecomment-1941488430)
Concept ACK

💬 Sjors commented on pull request "Add Signet launch shortcut for Windows":
(https://github.com/bitcoin/bitcoin/pull/26334#issuecomment-1941502155)
Creating a shortcut is easy, but creating an icon with a non-confusing color is not.

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