💬 l0rinc commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1750222723)
> Also, changing the log severity is not a "refactor".

K, dropping this commit in that case

⚠️ ouziel-slama opened an issue: "No ZMQ `sequence` notifications on Regtest"
(https://github.com/bitcoin/bitcoin/issues/30853)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

No message received in the `sequence` feed when a transaction is added to the mempool.

### Expected behaviour

We should receive a ZMQ notification when a new transaction enters the mempool. This works fine on mainnet.

### Steps to reproduce

configure Bictoin Core with:
```
[regtest]
zmqpubrawtx=tcp://0.0.0.0:29332
zmqpubhashtx=tcp://0.0.0.0:29332
zmqpubsequence=tcp://0.0.0.0:29332
...

👍 real-or-random approved a pull request: "Update libsecp256k1 subtree to latest master"
(https://github.com/bitcoin/bitcoin/pull/30845#pullrequestreview-2289919879)
utACK https://github.com/bitcoin/bitcoin/pull/30845/commits/ff54395de421fa1cbcf80ddbca4d71f0d2a4d12f no blockers from libsecp256k1 side, and these commits touch only build/docs/tests and are thus particularly harmless

💬 pinheadmz commented on issue "No ZMQ `sequence` notifications on Regtest":
(https://github.com/bitcoin/bitcoin/issues/30853#issuecomment-2338139218)
There is a test for zmq sequence (`interface_zmq.py`) so it must be working on regtest. Compare your code to the `test_sequence()` block?

💬 furszy commented on pull request "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD":
(https://github.com/bitcoin/bitcoin/pull/30807#discussion_r1750286691)
> nit: Seems like this isn't needed so I would have preferred if that was left out or a separate commit because the file isn't touched at all otherwise.

This is needed to prevent a race condition. Since commit fafc73bc036792923ca64a112684abaf78f3b48e, `nLocalServices` can be accessed from different threads. Therefore, we need to store the value once it's read and use it here and few lines below (in the `InitializeNode` call), to maintain consistency in this flow. Otherwise, `use_v2transport`
...

💬 sipa commented on issue "28.0rc1 synchronizes much slower on Windows":
(https://github.com/bitcoin/bitcoin/issues/30833#issuecomment-2338162908)
> I wonder if this issue could be caused by number of calls we make to `fwrite` because of the write buffering in xor path of `AutoFile::write()`, maybe platforms other than windows are optimizing our writes into batches behind the scenes? I don't know much about how I/O works behind the scenes, just a guess.

I have not checked, but this would be my guess.

💬 furszy commented on pull request "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD":
(https://github.com/bitcoin/bitcoin/pull/30807#discussion_r1750305246)
> If this is self-contained and you need to wipe everything this could have been done in a separate test line `p2p_assumeutxo.py` for example and you copy over the setup from this file here but it seems like there isn't much needed.

I started there (see me crying about the `feature_assumeutxo.py` code organization here https://github.com/bitcoin/bitcoin/pull/30807#pullrequestreview-2281468112), but ended up moving it inside this all-in-one file because I wasn't happy copy pasting the costly
...

🤔 l0rinc reviewed a pull request: "fix: avoid calling `GetCoin` and `SignTransaction()` inside of `assert(...)` in tests"
(https://github.com/bitcoin/bitcoin/pull/29572#pullrequestreview-2289988052)
I'd prefix the PR with `test:` to obviate that it doesn't touch production code.
And the PR description isn't convincing enough to me.

💬 l0rinc commented on pull request "fix: avoid calling `GetCoin` and `SignTransaction()` inside of `assert(...)` in tests":
(https://github.com/bitcoin/bitcoin/pull/29572#discussion_r1750307637)
This was addressed in https://github.com/bitcoin/bitcoin/pull/30849/files#diff-6a8ef76c60f30a6ca67d9f0e478fd02989c4b7fbc4c3116f80e13d873d5775e6R454 without an assert/Assert

💬 fanquake commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2338212801)
Guix build (x86_64) note that macOS builds (probably) wont match:
```bash
b3bcd9f7508b35b3f4a187e9f1e4c87648bf3897575c0c4db8664f1fe08c2cf4 guix-build-be4f78275fa6/output/aarch64-linux-gnu/SHA256SUMS.part
356c7b9493887fc839e6b7951a3396085bfea33efce91070f972a69f60aac1f4 guix-build-be4f78275fa6/output/aarch64-linux-gnu/bitcoin-be4f78275fa6-aarch64-linux-gnu-debug.tar.gz
e2887613ca5d1f929294487f86bd36ab6af0739d12625c6c661a35781c4f1523 guix-build-be4f78275fa6/output/aarch64-linux-gnu/bitcoin-b
...

💬 fanquake commented on pull request "Update libsecp256k1 subtree to latest master":
(https://github.com/bitcoin/bitcoin/pull/30845#issuecomment-2338219423)
Guix Build (aarch64) note that macOS builds (probably) wont match:
```bash
d5dce4d500967a50a4f5717aabb50bac198ce3a1524b8d8bfdfc015a7bc2ddd4 guix-build-ff54395de421/output/aarch64-linux-gnu/SHA256SUMS.part
90eaee7f778ed489884dccdc049341ee218372aaf782c6ed43a503d16433c34f guix-build-ff54395de421/output/aarch64-linux-gnu/bitcoin-ff54395de421-aarch64-linux-gnu-debug.tar.gz
10190f271c5181c9c956fd36aee9f22f36c80ca844a805902bbc0a078ca0fe56 guix-build-ff54395de421/output/aarch64-linux-gnu/bitcoin-
...

💬 fjahr commented on pull request "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD":
(https://github.com/bitcoin/bitcoin/pull/30807#discussion_r1750343042)
> I think we should allow assumeUTXO snapshot customization on regtest. It would make testing much simpler and self-contained.

How would it make it simpler? It's another thing to keep track of in the test code and the implementation code. If we need more snapshot heights we can always add them but I haven't felt the need for it so far when I have added new coverage.

💬 TheCharlatan commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2338228584)
Guix builds (aarch64):
```
b3bcd9f7508b35b3f4a187e9f1e4c87648bf3897575c0c4db8664f1fe08c2cf4 guix-build-be4f78275fa6/output/aarch64-linux-gnu/SHA256SUMS.part
356c7b9493887fc839e6b7951a3396085bfea33efce91070f972a69f60aac1f4 guix-build-be4f78275fa6/output/aarch64-linux-gnu/bitcoin-be4f78275fa6-aarch64-linux-gnu-debug.tar.gz
e2887613ca5d1f929294487f86bd36ab6af0739d12625c6c661a35781c4f1523 guix-build-be4f78275fa6/output/aarch64-linux-gnu/bitcoin-be4f78275fa6-aarch64-linux-gnu.tar.gz
92514b096
...

💬 furszy commented on pull request "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD":
(https://github.com/bitcoin/bitcoin/pull/30807#issuecomment-2338236543)
Thanks for the review fjahr!, it seems that your scripted-diff commit https://github.com/fjahr/bitcoin/commit/6cc25ffb1411285d8191bc932eb37b96348f022c makes CI unhappy. I haven't checked the reason but we could leave it for a follow-up to move forward with the fix for v28. Happy to check it.

✅ UdjinM6 closed a pull request: "fix: avoid calling `GetCoin` and `SignTransaction()` inside of `assert(...)` in tests"
(https://github.com/bitcoin/bitcoin/pull/29572)

💬 maflcko commented on issue "28.0rc1 synchronizes much slower on Windows":
(https://github.com/bitcoin/bitcoin/issues/30833#issuecomment-2338250464)
The xor patch didn't change buffering. Yes, in theory there is a 4096 byte max buffer, however, this is never hit in practice. Undo data consists of small integers (4 bytes or 1 byte) and short strings (like output scripts).

I expect that if you print the number of writes before and after, they are exactly the same. If you also print the size of the writes they should show a histogram where 1-byte writes are the most common (probably all varint), followed by various writes all shorter than 33
...

👍 TheCharlatan approved a pull request: "security-check: test for `_FORTIFY_SOURCE` usage in release binaries"
(https://github.com/bitcoin/bitcoin/pull/27038#pullrequestreview-2290087577)
ACK be4f78275fa6608b11377dd5a29a809597d3fe8d

💬 TheCharlatan commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#discussion_r1750369033)
Bit unfortunate that this list has to be maintained, but I can't think of a better way either.

👍 fanquake approved a pull request: "Update libsecp256k1 subtree to latest master"
(https://github.com/bitcoin/bitcoin/pull/30845#pullrequestreview-2290092420)
ACK ff54395de421fa1cbcf80ddbca4d71f0d2a4d12f

💬 ryanofsky commented on pull request "refactor: Generalize `CScript`'s `std::vector` push to accept `std::array`":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750230284)
In commit "refactor: Make code more tolerant of constexpr std::arrays" (c4c102de7336e6c0cc2e5187feedf437ebadb565)

I'm not sure I see benefit of changing this from `unsigned char` to `uint8_t` now when there's a good chance this is going to be switched again to `std::byte` in the future. It could also be good eliminate the C cast on this line if it is changing anyway.

So not important, but would consider changing `uint8_t(size)` to `static_cast<value_type(size)>` here and changing `uint8_t`
...