💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#issuecomment-2165748009)
@theStack part of splitting out the diagram stuff to a prep PR is that you can just assume it means "economically rational to replace" :+1:

💬 andrewtoth commented on pull request "validation: sync chainstate to disk after syncing to tip":
(https://github.com/bitcoin/bitcoin/pull/15218#discussion_r1638266045)
> Isn't this going to always reschedule the task on the first run?

Yes, but do you think this is a problem? It just makes sure the node has not connected any nodes for at least 30 seconds.

> Also, the active height refers to the latest connected block. It doesn't tell us we are up-to-date with the network; To know if we are sync, should use the best known header or call to the ApproximateBestBlockDepth() function.

Doesn't the fact that `IsInitialBlockDownload()` returns false make this
...

📝 theuni opened a pull request: "upnp: fix build with miniupnpc 2.1.8"
(https://github.com/bitcoin/bitcoin/pull/30283)
Fixes https://github.com/bitcoin/bitcoin/issues/30266

Miniupnpc 2.1.8 [changed the function signature of `UPNP_GetValidIGD`](https://github.com/miniupnp/miniupnp/commit/c0a50ce33e3b99ce8a96fd43049bb5b53ffac62f#diff-5a0d7cff00628c2c64a617edb347c0f283e3a75e7df910e7e8438fc6db23f610R122) without taking much care with the abi :(

This is the minimal change to cope with that. Also included in this PR is a temporary bump to 2.1.8 to verify that it builds correctly. I'm happy to revert that and dis
...

💬 ismaelsadeeq commented on pull request "rename TransactionErrors: MISSING_INPUTS and ALREADY_IN_CHAIN":
(https://github.com/bitcoin/bitcoin/pull/30212#discussion_r1638282582)
> Therefore as an "internal error", TX_MISSING_INPUTS (for any reason) it felt more accurate to me to leave as is?

I was reviewing this based on the PR rationale in the OP. You stated that "a transaction's inputs are missing from the UTXO set, which could also be due to all inputs having already been spent (MISSING_INPUTS -> INPUTS_MISSING_OR_SPENT)." That's why I suggested that `TxValidationResult` one should be changed just for uniformity. But Internally, you're right `TX_MISSING_INPUTS` i
...

💬 theuni commented on pull request "upnp: fix build with miniupnpc 2.2.8":
(https://github.com/bitcoin/bitcoin/pull/30283#issuecomment-2165781468)
Whoops, fixed typo. s/2.1.8/2.2.8/ in several places.

💬 instagibbs commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#issuecomment-2165812047)
reACK https://github.com/bitcoin/bitcoin/pull/30111/commits/0e0c422aedd4009ab34eca127e4904d15e81f5be

just a rebase

reviewed via `git range-diff master a9a6de5 0e0c422`

👍 TheCharlatan approved a pull request: "Encapsulate warnings in generalized node::Warnings and remove globals"
(https://github.com/bitcoin/bitcoin/pull/30058#pullrequestreview-2115950869)
Re-ACK 260f8da71a35232d859d7705861fc1a88bfbbe81

👍 theuni approved a pull request: "Update leveldb subtree to latest upstream"
(https://github.com/bitcoin/bitcoin/pull/30281#pullrequestreview-2115953719)
utACK 95812d912b6335caa7af2a084d84447fb4aad156

💬 Sjors commented on pull request "Introduce Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30200#issuecomment-2165900439)
Thanks for the review, and for the suggestions. I might add that later to this PR or as a followup.

Trivial rebase after #29015.

🤔 ismaelsadeeq reviewed a pull request: "doc: use TRUC instead of v3 and add release note"
(https://github.com/bitcoin/bitcoin/pull/30272#pullrequestreview-2116107521)
Concept ACK

👍 BrandonOdiwuor approved a pull request: "test: cover more errors for `signrawtransactionwithkey` RPC"
(https://github.com/bitcoin/bitcoin/pull/30278#pullrequestreview-2116157106)
Code Review ACK e2779ce98b39e14cada08a654928e798436f5a46

💬 achow101 commented on pull request "refactor: Reduce memory copying operations in bech32 encoding":
(https://github.com/bitcoin/bitcoin/pull/29607#issuecomment-2166121357)
ACK 07f64177a49f1b6b4d486d10cf67fddfa3c995eb

💬 achow101 commented on pull request "netbase: extend CreateSock() to support creating arbitrary sockets":
(https://github.com/bitcoin/bitcoin/pull/30202#issuecomment-2166141888)
ACK 5f549c35d9a75c7019fe8a96963b988df5498eed

🚀 achow101 merged a pull request: "refactor: Reduce memory copying operations in bech32 encoding"
(https://github.com/bitcoin/bitcoin/pull/29607)

💬 achow101 commented on pull request "netbase: extend CreateSock() to support creating arbitrary sockets":
(https://github.com/bitcoin/bitcoin/pull/30202#issuecomment-2166154443)
Silent merge conflict

```
../../../src/test/fuzz/i2p.cpp:30:16: error: no viable overloaded '='
30 | CreateSock = [&fuzzed_data_provider](const sa_family_t&) {
| ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
31 | return std::make_unique<FuzzedSock>(fuzzed_data_provider);
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
32 | };
| ~
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/14.1.1/../../../../include/c++
...

💬 paplorinc commented on pull request "refactor: Reduce memory copying operations in bech32 encoding":
(https://github.com/bitcoin/bitcoin/pull/29607#issuecomment-2166248133)
Thanks for the reviews @josibake, @sipa, @achow101.
There's a similar small optimization for [transaction input/output allocations](https://github.com/bitcoin/bitcoin/pull/30093) as well, I'd appreciate a review.

💬 davidgumberg commented on pull request "Lint: improve subtree exclusion":
(https://github.com/bitcoin/bitcoin/pull/29965#issuecomment-2166387546)
Rebased over master since #30219 was merged.

💬 mzumsande commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1638706909)
Some checks are within `ActivateSnapshot()`, others, like the added one, are in the rpc code (which means that they can't be covered in unit tests for that function). Do you know if there is a reason for that?

💬 ryanofsky commented on pull request "Multiprocess bitcoin":
(https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-2166556815)
re: https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-2158932014

> It is possible there is a bug here, though so if you want to post an issue to the libmultiprocess github project so I can look into it more, that would be helpful.

To follow up on this, I actually found two separate bugs which were causing wallet objects not to be destroyed when the node process is killed. Coincidentally a new test was added recently in #26606 which reproduces this problem, because it actually test
...

👍 ryanofsky approved a pull request: "Introduce Mining interface"
(https://github.com/bitcoin/bitcoin/pull/30200#pullrequestreview-2116692613)
Code review ACK 5666af2c6e4924bf8e358ab5121c88d14cb085df, just rebased since last review