💬 hebasto commented on pull request "[DO NOT MERGE] cmake: Migrate CI scripts to CMake-based build system -- WIP":
(https://github.com/bitcoin/bitcoin/pull/29790#issuecomment-2122500393)
@maflcko

The last two commits in this branch have been cherry-picked into https://github.com/hebasto/bitcoin/pull/177.

Would you mind leave your comment in there please?

💬 fjahr commented on pull request "rpc: Optimize serialization and enhance metadata of dumptxoutset output":
(https://github.com/bitcoin/bitcoin/pull/29612#discussion_r1608225552)
done

💬 fjahr commented on pull request "rpc: Optimize serialization and enhance metadata of dumptxoutset output":
(https://github.com/bitcoin/bitcoin/pull/29612#discussion_r1608225734)
Yeah, seems so, done

💬 fjahr commented on pull request "rpc: Optimize serialization and enhance metadata of dumptxoutset output":
(https://github.com/bitcoin/bitcoin/pull/29612#discussion_r1608225976)
done

💬 fjahr commented on pull request "rpc: Optimize serialization and enhance metadata of dumptxoutset output":
(https://github.com/bitcoin/bitcoin/pull/29612#discussion_r1608226073)
done

💬 fjahr commented on pull request "rpc: Optimize serialization and enhance metadata of dumptxoutset output":
(https://github.com/bitcoin/bitcoin/pull/29612#discussion_r1608226272)
done

💬 fjahr commented on pull request "rpc: Optimize serialization and enhance metadata of dumptxoutset output":
(https://github.com/bitcoin/bitcoin/pull/29612#discussion_r1608226361)
Cool, done!

💬 fjahr commented on pull request "rpc: Optimize serialization and enhance metadata of dumptxoutset output":
(https://github.com/bitcoin/bitcoin/pull/29612#issuecomment-2122506010)
Addressed feedback from @TheCharlatan and @theStack , thank you!

💬 hebasto commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-2122512762)
My Guix build:
```
07318f5568a25cf18d6ec29d8fbaba3adc9ddfa23891fbb62d63e136b5d25c7f guix-build-1c26d10b23bb/output/aarch64-linux-gnu/SHA256SUMS.part
acbc7851322d559b96cca83764ef32f6e7df0f2449b1ca951a54127efea8eaf3 guix-build-1c26d10b23bb/output/aarch64-linux-gnu/bitcoin-1c26d10b23bb-aarch64-linux-gnu-debug.tar.gz
c94cdf25eab02a297f5b25fed2211cf0b35f2b1b6c7d48b154f2ed46720b9abb guix-build-1c26d10b23bb/output/aarch64-linux-gnu/bitcoin-1c26d10b23bb-aarch64-linux-gnu.tar.gz
2a7aca0c44b234af3
...

💬 fjahr commented on pull request "assumeutxo, rpc: Improve EOF error when reading snapshot metadata in loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/28670#issuecomment-2122525863)
Hm, unfortunately, I think this PR was made redundant by #29612 recently. After the latest feedback there which I addressed just 3 weeks ago, we are enhancing the assumeutxo metadata there significantly and this means all of the lines changed here are touched there as well and I don't see much of an overlap either.

💬 fanquake commented on pull request "util: avoid using thread_local variable that has a destructor":
(https://github.com/bitcoin/bitcoin/pull/30095#discussion_r1608252026)
This was already backported to the 13 and 14 branches, but we could add another comment here.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1608255803)
Yes, `iters->by_txid->first` is the same as `txid`, changed it to use `txid` which is shorter.

It is necessary to remove and re-add because we have changed the key and C++ maps do not support modifying of the keys. That is, changing the key means that the entry will have to be in another place in the map, so it has to be removed from the old place and added to the new one.

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2122543090)
> @laanwj This is a router provided by Verizon (a large US ISP) in 2021.

That's curious, especially as NAT-PMP doesn't have any IPv6 support. i'm assuming you don't have any output for IPv6?

💬 glozow commented on pull request "refactor: TxDownloadManager":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1608263692)
I don't think we'd ever need to make `TxDownloadManager` depend on `ChainstateManager` as we could just pass in the block hash, i.e.

> Another alternative could be to directly pass the current block hash as a param into the relevant TxDownloadManager methods.

This would avoid the addition of `UpdateBlockTipSync` etc, as it's basically what we do now. I find this way of keeping synchronized with the chain tip pretty ugly - we'd need to hold `cs_main` and pass the blockhash to the `TxDownloa
...

👍 hebasto approved a pull request: "build: Remove `--enable-threadlocal`"
(https://github.com/bitcoin/bitcoin/pull/30137#pullrequestreview-2068533600)
ACK 17fe948cce2eb75f0f3f4b0db9d0d90648c7d4af.

Guix build:
```
x86_64
b5dd1ec801cb549590bb922867b9254de51c3688220ce4d29ae03077e0047448 guix-build-17fe948cce2e/output/dist-archive/bitcoin-17fe948cce2e.tar.gz
9bc0b56bf0440f04937646ded534e47df860c4ca2b9b96699c1f5b431ae71508 guix-build-17fe948cce2e/output/x86_64-w64-mingw32/SHA256SUMS.part
3e1cceb01e86b7e47d68ae100b60747333a88aad5b352c7c3e9113d700465f1c guix-build-17fe948cce2e/output/x86_64-w64-mingw32/bitcoin-17fe948cce2e-win64-debug.zip
...

💬 maflcko commented on pull request "util: avoid using thread_local variable that has a destructor":
(https://github.com/bitcoin/bitcoin/pull/30095#discussion_r1608266669)
Ok, so assuming this was fixed, it can probably be reverted some time next year, given the EOL policy https://www.freebsd.org/security/#sup

💬 glozow commented on pull request "refactor: TxDownloadManager":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1608272072)
Hm, in the previous setup, we can change any of txdownload_impl.cpp without net_processing.cpp noticing, but yeah maybe less clean to depend on txdownload_impl.h.

I've updated with a new setup where I've added a txdownloadman.cpp depending on txdownload_impl.h, and txdownloadman.h is included by txdownload_impl.h instead of the other way around. And so net_processing.cpp no longer depends on txdownload_impl.h, which makes sense to me. Had to update `lint-circular-dependencies.py` but I do lik
...

🤔 fjahr reviewed a pull request: "net: add ASMap info in `getrawaddrman` RPC"
(https://github.com/bitcoin/bitcoin/pull/30062#pullrequestreview-2068538362)
Concept ACK

💬 fjahr commented on pull request "net: add ASMap info in `getrawaddrman` RPC":
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1608272490)
nit, I think this wording would be more clear: "The ASN mapped to the IP of this peer per our current ASMap." (and something corresponding to this below).

💬 fjahr commented on pull request "net: add ASMap info in `getrawaddrman` RPC":
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1608267929)
I would prefer these to be optional results rather than a magic 0 value.