💬 mzumsande commented on pull request "addrman: delete addresses that don't belong to the supported networks":
(https://github.com/bitcoin/bitcoin/pull/29330#issuecomment-1930512296)
> Deleting addresses that do not belong to the supported networks can avoid a lot of unnecessary addrman `Select` calls

It would be good to have some intution for how big this effect is. For example, if you have an addrman with mostly IPv4/IPv6 addresses and a few onion ones then restart with `-onlynet=onion`, will it take noticeably longer to find peers? The worst case scenario would be if you had just a single onion address, but that doesn't seem realistic, so I'd be more interested about t
...

👍 Empact approved a pull request: "test: Fix CPartialMerkleTree.nTransactions signedness"
(https://github.com/bitcoin/bitcoin/pull/29363#pullrequestreview-1866035101)
ACK facafa90f7a1eee452f9baf8a1b65a2edac0982b

💬 achow101 commented on pull request "wallet: refactor: remove unused `SignatureData` instances in spkm's `FillPSBT` methods":
(https://github.com/bitcoin/bitcoin/pull/28833#issuecomment-1930518879)
ACK e2ad343f69af4f924b22dccf94a52b6431ef6e3c

💬 theStack commented on pull request "test: p2p: adhere to typical VERSION message protocol flow":
(https://github.com/bitcoin/bitcoin/pull/29353#issuecomment-1930522226)
> As a side note, I found the newly added comments regarding inbounds and outbounds to be a bit confusing. This is due to them being written from the POV of the Python node instead of the TestNode's, which is the logic used by `add_p2p_connection` and `add_outbound_p2p_connection`

I agree that the inbound/outbound naming is currently not ideal w.r.t. mixed node perspectives and should be improved. Will look into a potential follow-up PR addressing this (https://github.com/bitcoin/bitcoin/pull
...

🚀 achow101 merged a pull request: "wallet: refactor: remove unused `SignatureData` instances in spkm's `FillPSBT` methods"
(https://github.com/bitcoin/bitcoin/pull/28833)

💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1480392199)
Only if you do further touchups, but if so, perhaps you can add a test here that if a solution was returned, even if `GetAlgoCompleted` isn't true, that `result_cg->GetWeight() <= high_max_weight` and that `result_cg->GetSelectedEffectiveValue() <= target + coin_params.m_min_change_target`?

Also `best_weight > result_cg->GetWeight() || (best_weight == result_cg->GetWeight() && best_amount >= result_cg->GetSelectedEffectiveValue())` would be possible to test, though adds less (it failing would
...

💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1480405237)
`std::ceil` doesn't do anything here, the input is already a rounded-down integer. The proper way to compute a rounding-up integer division $\lceil\frac{a}{b}\rceil$ is `(a + b - 1) / b`.

Also, can you drop the `!best_selection.empty() &&` here?

💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1480406547)
This test is modified in the last commit; it'd be more convincingly improving runtime if the final version of the test was already included in the commit that initially adds it.

🤔 sr-gi reviewed a pull request: "test: maxuploadtarget: check for mempool msg disconnect if limit is reached, improve existing test coverage"
(https://github.com/bitcoin/bitcoin/pull/28996#pullrequestreview-1866063353)
tack [b58f009](https://github.com/bitcoin/bitcoin/commit/b58f009d9585aab775998644de07e27e2a4a8045)

💬 sr-gi commented on pull request "test: maxuploadtarget: check for mempool msg disconnect if limit is reached, improve existing test coverage":
(https://github.com/bitcoin/bitcoin/pull/28996#discussion_r1480385304)
This is sort of unrelated, but it may be worth bringing it in given we are updating this test anyway. The line right above this mentions that the time is advanced by 24 hours and then the checks are performed again. However, the time is actually advance 48 hours: it was previously set to 48 hours in the past, and now it's set to the current time.

I guess it wouldn't hurt to also define `SECONDS_PER_DAY = 60*60*24` for readability throughout the test.

💬 sr-gi commented on pull request "test: maxuploadtarget: check for mempool msg disconnect if limit is reached, improve existing test coverage":
(https://github.com/bitcoin/bitcoin/pull/28996#discussion_r1480406396)
nit: whitespace + caps here

💬 mzumsande commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1480436573)
> do you have a code path where nChainTx is read when it was wrong (previously) or is unset (now)

no, I never understood how the fake `nChainTx` values would help with guessing progress, but also wasn't 100% sure.

💬 ryanofsky commented on issue "Update nChainTx to 64bit type":
(https://github.com/bitcoin/bitcoin/issues/29258#issuecomment-1930671864)
> This leads to an alternative solution to not increase the memory footprint: Move the 32+32 bits used for nTx and nChainTx into a single 64-bit blob, where the 16 bit are for nTx and the remaining for nChainTx. But, again, the overall heap memory usage shouldn't change much, either way.

This does seem like an nice and easy solution. Would be good to incorporate in #29331

As a tangent, I was thinking along the same lines last week before implementing #29370 and came up with worse solution,
...

💬 epiccurious commented on pull request "i2p: log connection was refused due to arbitrary port":
(https://github.com/bitcoin/bitcoin/pull/29393#discussion_r1480491658)
Question on your approach here - why drop the `try .. catch ... if not raised` instead of just replacing line 27?

💬 epiccurious commented on pull request "i2p: log connection was refused due to arbitrary port":
(https://github.com/bitcoin/bitcoin/pull/29393#issuecomment-1930691538)
Concept ACK 950f3602d61f51d3c42c051ed9139916f1ee23bd.

💬 brunoerg commented on pull request "i2p: log connection was refused due to arbitrary port":
(https://github.com/bitcoin/bitcoin/pull/29393#discussion_r1480499623)
Because they are not necessary anymore. That try catch was a hack to check the logs because we didn't have a direct way to check in the logs that the reason was the port.

🤔 furszy reviewed a pull request: "sqlite: Disallow writing from multiple `SQLiteBatch`s"
(https://github.com/bitcoin/bitcoin/pull/29112#pullrequestreview-1866399622)
ACK cfcb9b1

💬 achow101 commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1480562396)
In f770aff87279ba252f7b8fcadef0e9d1828ca031 "test: Add coin_grinder_tests"

This check implies that there is an exact solution at 2041 attempts, and we should be able to know what that solution is, so can we check for it?

💬 achow101 commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1480563895)
In 3e2a97bd4bb275e2f0827a3099a64222c6460ed9 "coinselection: Track whether CG completed"

style nit: Open brace `{` should be on a new line for functions.

💬 achow101 commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1480554649)
In f770aff87279ba252f7b8fcadef0e9d1828ca031 "test: Add coin_grinder_tests"

nit: A comment with the weight calculation like the ones in later test would be nice.