💬 ryanofsky commented on pull request "init: settings, do not load auto-generated warning msg":
(https://github.com/bitcoin/bitcoin/pull/29301#discussion_r1480187026)
re: https://github.com/bitcoin/bitcoin/pull/29301#discussion_r1465379877

> As the settings map take strings for keys, wouldn't this change mean that we would need to static cast string_view to string to not create a copy during the push/erase methods call?

The idea is that just that it is noticeable when programs are slow to start, so you usually want to reduce the amount of code that needs to run before main(), even if it means code that runs later but is less sensitive might be slower. I
...

💬 josibake commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#issuecomment-1930372190)
Concept ACK

Started reviewing, looks good. Will continue review tomorrow. Might be worth updating the last part of the description with the insights from https://github.com/bitcoin/bitcoin/pull/29112#issuecomment-1874716791 ?

💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1480260826)
@murchandamus Let me elaborate for context, but no point in addressing this in this PR (after my edit, I don't think it's a particularly impactful improvement even).

Let's say the transactions, in order after sorting are A,B,C,D,E,F. Imagine we've just processed `_BC`, and SHIFT after it (so, without clone skipping, the next exploration would be `_B_D`). If it happens to be the case that D is unambiguously worse than C, then D can be skipped as well. This happens when value(D)=value(C) AND we
...

💬 achow101 commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1480301257)
Moved it.

💬 achow101 commented on pull request "wallet: remove unused 'accept_no_keys' arg from decryption process":
(https://github.com/bitcoin/bitcoin/pull/29375#issuecomment-1930470491)
ACK 2bb25ce5023c4d56c8b11e9c75f9f8bd69894452

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

📝 brunoerg opened a pull request: "i2p: log connection was refused due to arbitrary port"
(https://github.com/bitcoin/bitcoin/pull/29393)
For I2P, we do not try to connect if port is != 0. However, we do not have anything that indicates it or any error when trying to connect with port != 0. This PR adds a log for it. Also, it improves the functional test. With this log we can ensure the reason we won't connect is the port, in the current test, we cannot ensure it.

🚀 achow101 merged a pull request: "wallet: remove unused 'accept_no_keys' arg from decryption process"
(https://github.com/bitcoin/bitcoin/pull/29375)

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