💬 benthecarman commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1556271482)
> Approach NACK.
>
> I get this - wasting time waiting for confirmations is more than annoying. Its effect usually lingers, because you loose context in the meantime and it slows down development in your entire organization. However, I also think supporting options that might lead to clients running disagreeing consensus rules on the same network is dangerous and might annoy developers in fresh ways. I would support the approach laid out by ajtowns in [this comment](https://github.com/bitcoin
...

💬 mzumsande commented on pull request "index: improve initialization and pruning violation check":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1199824807)
> Do you think the the main benefit of this commit is cleaner code, and the worse error detection a side effect? Or are there other benefits to this commit?

I think one minor downside could be that the indexes will be available a few minutes later because loadblk doesn't only do reindexing etc., it also loads the mempool - this could take several minutes depending on mempool size and there isn't really a reason why the indexes shouldn't be available before that has finished. This is already t
...

🤔 mzumsande reviewed a pull request: "Parallel compact block downloads, take 3"
(https://github.com/bitcoin/bitcoin/pull/27626#pullrequestreview-1435468460)
Concept ACK, code looks good on first walkthrough.

I want to run this code with some additional logging for a day or two before acking.

In particular, I wonder if the fact that we'll now try additional peers will lead to redundancies / increased traffic under non-adversarial conditions where there's no staller but delivering the block just needs some time - especially when blocks contain a large number of txns that weren't in our mempool, or on slower networks like Tor. Did you consider th
...

💬 mzumsande commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1199689963)
missing `assert` here

💬 mzumsande commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1199822582)
nit: `already_in_flight` could be just a bool here

🤔 mzumsande reviewed a pull request: "rpc: Add importmempool RPC"
(https://github.com/bitcoin/bitcoin/pull/27460#pullrequestreview-1435717252)
Should the rpc be executable before the mempool load from `ThreadImport` has finished? I tried importing another mempool before the mempool from the datadir was being loaded, and I didn't encounter any problems on first sight - but I'm not sure how useful that would be and I guess the final result would be dependent on races between the two mempools being loaded in parallel?

💬 furszy commented on pull request "index: improve initialization and pruning violation check":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1199939444)
> This is already the case now in case of -reindex-chainstate after https://github.com/bitcoin/bitcoin/pull/25193 (I didn't think of that), but it could be easily changed in master by moving g_indexes_ready_to_sync = true up one line in ThreadImport.

yeah, I don't see why that would be a downside of this changes neither. Same as with `g_indexes_ready_to_sync` flag, the `StartIndexes()` call could be executed before the mempool load too.

It wouldn't be bad to rename `ThreadImport` to `Impo
...

💬 jarolrod commented on pull request "build: Do not define `ENABLE_ZMQ` when ZMQ is not available":
(https://github.com/bitcoin/bitcoin/pull/27696#issuecomment-1556549338)
hashes:

```
7091c64cada36ec2850f889978ae5fdc3cf4f28ec40bfb1fd16100c3f502fe6e guix-build-fa5831bd6f94/output/aarch64-linux-gnu/SHA256SUMS.part
e3b87e22e4cbd7a50aac4d5c982c12dd5b3f224ff4f9543dd3c331d424899287 guix-build-fa5831bd6f94/output/aarch64-linux-gnu/bitcoin-fa5831bd6f94-aarch64-linux-gnu-debug.tar.gz
96ffa1075a4b1d14c842bb8a5f5106cdb8f401eb173578c7376f6454b918e4eb guix-build-fa5831bd6f94/output/aarch64-linux-gnu/bitcoin-fa5831bd6f94-aarch64-linux-gnu.tar.gz
1bbe3a56784762b811eadc
...

👍 jarolrod approved a pull request: "build: Do not define `ENABLE_ZMQ` when ZMQ is not available"
(https://github.com/bitcoin/bitcoin/pull/27696#pullrequestreview-1435784103)
ACK fa5831bd6f940c4afb43ff625ba4fa6c641e999a

💬 ajtowns commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#issuecomment-1556553095)
> In particular, I wonder if the fact that we'll now try additional peers will lead to redundancies / increased traffic under non-adversarial conditions where there's no staller but delivering the block just needs some time - especially when blocks contain a large number of txns that weren't in our mempool, or on slower networks like Tor. Did you consider the possibility of giving the original peer a few seconds to deliver before requesting from additional peers?

Yeah, this is the behaviour I
...

👍 MarcoFalke approved a pull request: "ci, iwyu: Double maximum line length for includes"
(https://github.com/bitcoin/bitcoin/pull/27707#pullrequestreview-1435841742)
lgtm

💬 MarcoFalke commented on pull request "ci, iwyu: Update mappings":
(https://github.com/bitcoin/bitcoin/pull/27710#issuecomment-1556608479)
concept ACK

🤔 MarcoFalke reviewed a pull request: "test: Explicitly specify directory where to search tests for"
(https://github.com/bitcoin/bitcoin/pull/27561#pullrequestreview-1435948057)
lgtm ACK c44f3f231988dc05c4c7a8a96bc2e7b1a54da277 💸

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK c44f3f231988dc05
...

🤔 MarcoFalke reviewed a pull request: "kernel: Remove interface_ui, util/system from kernel library"
(https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1435962922)
nice ACK bbfbcd0360e486d47025a412f2c0f4e8535ab255 😇

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: nice ACK bbfbcd0360e486d4
...

💬 MarcoFalke commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1200080302)
nit in 4452707edec91c7d7991f486dd41ef3edb4f7fbf: not sure what the convention is, but most other places seem to use `Untranslated("")` over this?

💬 MarcoFalke commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1200080447)
same?

💬 ajtowns commented on issue "Enable consistency checks by default with `--enable-debug`":
(https://github.com/bitcoin/bitcoin/issues/24709#issuecomment-1556705320)
Linux distros do detached debug symbols so that you can grab them after the fact and just use them with your regular production binaries. cf http://debug.mirrors.debian.org/debian-debug/pool/main/b/bitcoin/ Doing that for our guix builds might be worthwhile, in which case perhaps we could always build with debug symbols?

Thinking about it more, I think for me the levels are more something like:

* production -- should only abort on unrecoverable errors, should be as fast as possible
* de
...

🚀 fanquake merged a pull request: "ci, iwyu: Double maximum line length for includes"
(https://github.com/bitcoin/bitcoin/pull/27707)

💬 hebasto commented on pull request "ci, iwyu: Update mappings":
(https://github.com/bitcoin/bitcoin/pull/27710#issuecomment-1556804109)
Rebased on top of the merged https://github.com/bitcoin/bitcoin/pull/27707.

🚀 fanquake merged a pull request: "ci: remove `RUN_SECURITY_TESTS`"
(https://github.com/bitcoin/bitcoin/pull/27683)

💬 fanquake commented on issue "Compute 'short id' when transaction joins mempool":
(https://github.com/bitcoin/bitcoin/issues/27706#issuecomment-1556818802)
Ok. Closing for now. Discussion can continue, but it's not clear why this is an issue. General questions /discussion can also be asked/happen in IRC etc.