💬 fanquake commented on pull request "cleanse: switch to SecureZeroMemory for Windows cross-compile":
(https://github.com/bitcoin/bitcoin/pull/26950#issuecomment-2431330789)
@real-or-random @laanwj's comment is mostly correct. iirc at the time this was one of a few MSVC specific defines we had left in the codebase. Since then others have been added, but mostly to just workaround, or ignore bugs in MSVC.

> (except perhaps performance if the implementation of SecureZeroMemory on mingw-w64 is worse than what the compiler can come up with for the memset).

I would think the code would not be worse performance wise, as it's implemented using `__stosb`.

💬 kristapsk commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#issuecomment-2431441971)
Rebased.

💬 vasild commented on issue "net: Tor service target port collides when running multiple nodes, making bitcoind error out":
(https://github.com/bitcoin/bitcoin/issues/31133#issuecomment-2431475513)
I am in favor of 2. (port+1) or 5. (do nothing) in the short term, for 28.1.

For long term, maybe deprecate and remove `port=` (yes, I think it is a subset of `bind=`, somebody correct me if I am wrong) in favor of `bind=...:port`. I agree that `port=` is "helpful, especially for users less knowledgeable about networking", maybe to resolve that extend the syntax of `bind=` to allow `bind=*:8333`. That is used in at least Apache configs: `<VirtualHost *:80>`.

💬 brunoerg commented on pull request "fuzz: wallet: add target for spkm migration":
(https://github.com/bitcoin/bitcoin/pull/29694#issuecomment-2431486918)
Inputs for this are available at: https://github.com/brunoerg/qa-assets/pull/2

💬 brunoerg commented on issue "TestFramework: TestShell.reset() will always fail":
(https://github.com/bitcoin/bitcoin/issues/31131#issuecomment-2431516183)
Thanks for it! I think this is similar to #30714 (which fixed the initialization).

💬 naiyoma commented on issue "rpc: show P2(W)SH redeemScript in getrawtransaction (and friends)":
(https://github.com/bitcoin/bitcoin/issues/27391#issuecomment-2431610508)
Picking this up since all previous attempts are now closed.

:lock: fanquake locked an issue: "COIN_VALUE no longer accessible in const contexts"
(https://github.com/bitcoin/bitcoin/issues/31113)

💬 brunoerg commented on pull request "Introduce `g_fuzzing` global for fuzzing checks":
(https://github.com/bitcoin/bitcoin/pull/31093#issuecomment-2431705621)
Concept ACK

🤔 stickies-v reviewed a pull request: "Drop miniupnp dependency"
(https://github.com/bitcoin/bitcoin/pull/31130#pullrequestreview-2388250835)
Concept ACK

👍 laanwj approved a pull request: "refactor: migrate `bool GetCoin` to return `optional<Coin>`"
(https://github.com/bitcoin/bitcoin/pull/30849#pullrequestreview-2388406290)
Code review ACK 4feaa28728442b0fd29a677d2b170a05fdf967c0
Not the easiest refactor to review, carefully checked that there are no behavior differences, but i think the change absolutely worth it for the more readable API, and the removal of ambiguity.

💬 ismaelsadeeq commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1812617677)
Yeah will break existing grepping scripts, but I think we will need to do this anyway to improve the logging for package replacements.

I think we should log the new package or new transaction replacement only once after logging all the replaced transactions:

1. First, log all replaced transactions with their ID, witness, fee, and size.
2. Then:
- For a single transaction replacement, log its ID, witness, fee, and size.
- For a package replacement, log that a new package has been
...

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-2431934694)
I will wait for some conceptual support before writing tests for `InputFetcher`.

💬 ismaelsadeeq commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1812631628)
Its not specified, I think we have to specify tracing api for packages;

Should be something like

#### Tracepoint `mempool:replaced_by_package`

Is called when a transaction in the node's mempool is getting replaced by a package.
Passes information about the replaced transaction and the replacement package.

Arguments passed:
1. Replaced transaction ID (hash) as `pointer to unsigned chars` (i.e. 32 bytes in little-endian)
2. Replaced transaction virtual size as `int32`
3. Replaced
...

💬 kevkevinpal commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#discussion_r1812631980)
Sounds good to me! Updated in [ad38457](https://github.com/bitcoin/bitcoin/pull/31040/commits/ad384572cad35a248ab0322e9f8855bbed024f0b)

💬 kristapsk commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2431955857)
Concept ACK

💬 sipa commented on pull request "rpc, cli: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31135#discussion_r1812643857)
Would it make sense to pass `chainman.m_best_header->nTime` to `GuessVerificationProgress()` (which then uses that instead of the current time)? That would make the result always consistently report progress w.r.t. the headers tip, instead of w.r.t. the current time.

💬 kevkevinpal commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2432008025)
Concept ACK

There have been instances where there have been breakage when using newer minor versions of miniupnp aswell to the other mentioned reasons
https://github.com/bitcoin/bitcoin/issues/30266 -> https://github.com/bitcoin/bitcoin/pull/30283 -> https://github.com/bitcoin/bitcoin/pull/30301

💬 kevkevinpal commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2432021055)
Might be worth adding a release note?

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812514096)
can we name the variable such that we don't need this comment?

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812439172)
what's the purpose of this block format?