💬 hebasto commented on pull request "QRImageWidget: Sizing and font fixups":
(https://github.com/bitcoin-core/gui/pull/506#issuecomment-1549832808)
This https://github.com/bitcoin-core/gui/pull/506#pullrequestreview-1043535557:
> This PR effectively renders font size smaller that makes it harder to read.
remains unaddressed.
(https://github.com/bitcoin-core/gui/pull/506#issuecomment-1549832808)
This https://github.com/bitcoin-core/gui/pull/506#pullrequestreview-1043535557:
> This PR effectively renders font size smaller that makes it harder to read.
remains unaddressed.
💬 Sjors commented on pull request "ConnectTip: don't log total disk read time in bench":
(https://github.com/bitcoin/bitcoin/pull/27673#discussion_r1195297945)
Happy to review such a change, but I barely remember making the original PR and am not very familiar with the bench logging.
(https://github.com/bitcoin/bitcoin/pull/27673#discussion_r1195297945)
Happy to review such a change, but I barely remember making the original PR and am not very familiar with the bench logging.
💬 glozow commented on pull request "rpc: allow submitpackage to be called outside of regtest":
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1549845292)
This wouldn't be backported. The first 2 commits could be considered a band aid "fix" but for a regtest-only RPC. The last 2 commits contain a new feature for miners, and we do not backport new features.
I understand this is a big limitation for Lightning, and I agree it should be treated with high priority. The way to fix it is to provide a way to submit and propagate packages to miners. If you would like to make package relay available in Bitcoin Core, please review #26711.
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1549845292)
This wouldn't be backported. The first 2 commits could be considered a band aid "fix" but for a regtest-only RPC. The last 2 commits contain a new feature for miners, and we do not backport new features.
I understand this is a big limitation for Lightning, and I agree it should be treated with high priority. The way to fix it is to provide a way to submit and propagate packages to miners. If you would like to make package relay available in Bitcoin Core, please review #26711.
👍 theStack approved a pull request: "Implement Mini version of BlockAssembler to calculate mining scores"
(https://github.com/bitcoin/bitcoin/pull/27021#pullrequestreview-1428805458)
Code-review ACK 6b605b91c1faf2c7f7cc0c9d39b4fcfd66dc2965
(https://github.com/bitcoin/bitcoin/pull/27021#pullrequestreview-1428805458)
Code-review ACK 6b605b91c1faf2c7f7cc0c9d39b4fcfd66dc2965
💬 kroese commented on issue "Allow getblockfrompeer to use any peer":
(https://github.com/bitcoin/bitcoin/issues/27652#issuecomment-1549867573)
> Well, peers could delay the response or directly not answer. Stalling the post-filter-matching blocks sync mechanism.
Yes, I think that peers who have reached their `maxuploadtarget` will also refuse to provide the block unless you have the whitelist / `download` permission. So there can be some edge-cases where non-pruned peers still fail to deliver it.
(https://github.com/bitcoin/bitcoin/issues/27652#issuecomment-1549867573)
> Well, peers could delay the response or directly not answer. Stalling the post-filter-matching blocks sync mechanism.
Yes, I think that peers who have reached their `maxuploadtarget` will also refuse to provide the block unless you have the whitelist / `download` permission. So there can be some edge-cases where non-pruned peers still fail to deliver it.
💬 joostjager commented on pull request "rpc: allow submitpackage to be called outside of regtest":
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1549871285)
Even just backporting the first two commits could be helpful, because it at least reduces the size of the patch that a miner needs to run.
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1549871285)
Even just backporting the first two commits could be helpful, because it at least reduces the size of the patch that a miner needs to run.
📝 ajtowns converted_to_draft a pull request: "net_processing: Drop m_recently_announced_invs bloom filter"
(https://github.com/bitcoin/bitcoin/pull/27675)
This PR replaces the `m_recently_announced_invs` bloom filter with a simple timestamp of the last time we considered sending an INV message to a node. This saves 33kB per peer (or more if we raise the rate at which we relay transactions over the network, in which case we would need to increase the size of the bloom filter proportionally).
The philosophy here (compare with #18861 and #19109) is that we consider the rate limiting on INV messages to only be about saving bandwidth and not protect
...
(https://github.com/bitcoin/bitcoin/pull/27675)
This PR replaces the `m_recently_announced_invs` bloom filter with a simple timestamp of the last time we considered sending an INV message to a node. This saves 33kB per peer (or more if we raise the rate at which we relay transactions over the network, in which case we would need to increase the size of the bloom filter proportionally).
The philosophy here (compare with #18861 and #19109) is that we consider the rate limiting on INV messages to only be about saving bandwidth and not protect
...
💬 MarcoFalke commented on pull request "net_processing: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1195220741)
nit: Looks like this will make inbound peers be treated identical to outbound ones for the purposes of getdata, as long as mapRelay is still around. For in-mempool txs it may be possible to fix by moving the time check outside of `info_for_relay` into this function and returning a nullptr early if the tx is known to not be announced. For non-mempool txs, I don't know.
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1195220741)
nit: Looks like this will make inbound peers be treated identical to outbound ones for the purposes of getdata, as long as mapRelay is still around. For in-mempool txs it may be possible to fix by moving the time check outside of `info_for_relay` into this function and returning a nullptr early if the tx is known to not be announced. For non-mempool txs, I don't know.
💬 MarcoFalke commented on pull request "net_processing: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1195225774)
nit: My recommendation would be to not rename the method, or if you want, to pick `GetEntryTime()` over `GetNodeTime()`, because the function doesn't return the current node time but the time of transaction entry.
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1195225774)
nit: My recommendation would be to not rename the method, or if you want, to pick `GetEntryTime()` over `GetNodeTime()`, because the function doesn't return the current node time but the time of transaction entry.
💬 jamesob commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#issuecomment-1549933311)
Thanks for the review! I'm done with nits on this one, and assumeutxo in general.
(https://github.com/bitcoin/bitcoin/pull/24008#issuecomment-1549933311)
Thanks for the review! I'm done with nits on this one, and assumeutxo in general.
📝 theuni opened a pull request: "macOS: Bump minimum required runtime version and prepare for building with upstream LLVM"
(https://github.com/bitcoin/bitcoin/pull/27676)
This (I believe) resolves the last of the blockers for [switching us away from cctools and instead using out-of-the-box llvm and lld](https://github.com/bitcoin/bitcoin/pull/21778) for building Darwin binaries.
For now, we continue building with a pre-packaged llvm and cctools, but after this PR the clang+lld combo should just work for anyone trying it. Additionally after this PR, the new runtime `fixup_chains` behavior will be in-use, as ld64 uses it as well.
The commits may seem unrelate
...
(https://github.com/bitcoin/bitcoin/pull/27676)
This (I believe) resolves the last of the blockers for [switching us away from cctools and instead using out-of-the-box llvm and lld](https://github.com/bitcoin/bitcoin/pull/21778) for building Darwin binaries.
For now, we continue building with a pre-packaged llvm and cctools, but after this PR the clang+lld combo should just work for anyone trying it. Additionally after this PR, the new runtime `fixup_chains` behavior will be in-use, as ld64 uses it as well.
The commits may seem unrelate
...
💬 achow101 commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#issuecomment-1549941663)
> This is probably not an issue, but it seems worth noting that a change of the underlying mock implementation had such an impact on the performance of legacy wallet loading.
It changed the underlying db used for this benchmark from the BDB in-memory database to the MockableDatabase. However I think the goal of this benchmark is to measure `LoadWallet` specifically and not necessarily the database engine, so I don't think it particularly matters.
(https://github.com/bitcoin/bitcoin/pull/26715#issuecomment-1549941663)
> This is probably not an issue, but it seems worth noting that a change of the underlying mock implementation had such an impact on the performance of legacy wallet loading.
It changed the underlying db used for this benchmark from the BDB in-memory database to the MockableDatabase. However I think the goal of this benchmark is to measure `LoadWallet` specifically and not necessarily the database engine, so I don't think it particularly matters.
💬 theuni commented on pull request "macOS: Bump minimum required runtime version and prepare for building with upstream LLVM":
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1549943895)
Ping @fanquake and @hebasto . There's not much happening here technically, but a few big-ish conceptual changes.
Also, we may want to go one step further in configure and do an actual compile check to verify that the minimum version is set to >=11.0 as we now have that as an assumption.
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1549943895)
Ping @fanquake and @hebasto . There's not much happening here technically, but a few big-ish conceptual changes.
Also, we may want to go one step further in configure and do an actual compile check to verify that the minimum version is set to >=11.0 as we now have that as an assumption.
💬 fanquake commented on pull request "macOS: Bump minimum required runtime version and prepare for building with upstream LLVM":
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1549945366)
Concept ACK - note that this also needs a change to `clang-toolchain-11` in Guix, which should be available without a time-machine change.
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1549945366)
Concept ACK - note that this also needs a change to `clang-toolchain-11` in Guix, which should be available without a time-machine change.
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1195390653)
The [docs](https://sqlite.org/c3ref/clear_bindings.html) don't say what the return values may be. I think SQLite itself will also log an error using the `ErrorLogCallback` that we during initialization.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1195390653)
The [docs](https://sqlite.org/c3ref/clear_bindings.html) don't say what the return values may be. I think SQLite itself will also log an error using the `ErrorLogCallback` that we during initialization.
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1195399114)
I don't think `upper_bound` or `equal_range` will work. They will find the upper bound to be the first item that is greater than our prefix, but any record with the right prefix and is longer than the prefix will be greater than it, so we end up actually only getting the record(s) that exactly match the prefix.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1195399114)
I don't think `upper_bound` or `equal_range` will work. They will find the upper bound to be the first item that is greater than our prefix, but any record with the right prefix and is longer than the prefix will be greater than it, so we end up actually only getting the record(s) that exactly match the prefix.
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1195406721)
Done
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1195406721)
Done
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1195406793)
Done
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1195406793)
Done
💬 hebasto commented on pull request "bench: Benchmark all `SHA256` implementations that are available on the system":
(https://github.com/bitcoin/bitcoin/pull/27598#issuecomment-1549995815)
Reworked according to @martinus comments.
(https://github.com/bitcoin/bitcoin/pull/27598#issuecomment-1549995815)
Reworked according to @martinus comments.
💬 hebasto commented on pull request "bench: Benchmark all `SHA256` implementations that are available on the system":
(https://github.com/bitcoin/bitcoin/pull/27598#discussion_r1195418857)
@martinus
Thanks! [Reworked](https://github.com/bitcoin/bitcoin/pull/27598#issuecomment-1549995815).
(https://github.com/bitcoin/bitcoin/pull/27598#discussion_r1195418857)
@martinus
Thanks! [Reworked](https://github.com/bitcoin/bitcoin/pull/27598#issuecomment-1549995815).