💬 maflcko commented on pull request "feature_taproot: sample tx version border values more":
(https://github.com/bitcoin/bitcoin/pull/32841#issuecomment-3027265968)
lgtm ACK 4be81e9746e9e18923386d6f4945a33885fd98a7
Also tested the failure on current master.
(https://github.com/bitcoin/bitcoin/pull/32841#issuecomment-3027265968)
lgtm ACK 4be81e9746e9e18923386d6f4945a33885fd98a7
Also tested the failure on current master.
📝 hebasto opened a pull request: "Update `minisketch` subtree and switch to its build script"
(https://github.com/bitcoin/bitcoin/pull/32856)
This PR:
1. Updates the `minisketch` subtree to latest upstream, which includes:
- https://github.com/bitcoin-core/minisketch/pull/75
2. Switches to `minisketch` upstream build system.
(https://github.com/bitcoin/bitcoin/pull/32856)
This PR:
1. Updates the `minisketch` subtree to latest upstream, which includes:
- https://github.com/bitcoin-core/minisketch/pull/75
2. Switches to `minisketch` upstream build system.
💬 hebasto commented on pull request "Update `minisketch` subtree and switch to its build script":
(https://github.com/bitcoin/bitcoin/pull/32856#issuecomment-3027293159)
This PR concludes the joint efforts of @sipa, @purpleKarrot, @theuni, and me.
(https://github.com/bitcoin/bitcoin/pull/32856#issuecomment-3027293159)
This PR concludes the joint efforts of @sipa, @purpleKarrot, @theuni, and me.
💬 Sjors commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3027353830)
Now that the wrapper binary landed in #31375 I think it's fine to ask users to use `bitcoin test` instead of `test_bitcoin`.
I agree `test_bitcoin` (and maybe `bench_bitcoin`) are useful enough to have them in the release.
But I doubt there are many _automated_ deployments out there that obtain the release binaries and run the tests. That's the only thing this PR would break.
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3027353830)
Now that the wrapper binary landed in #31375 I think it's fine to ask users to use `bitcoin test` instead of `test_bitcoin`.
I agree `test_bitcoin` (and maybe `bench_bitcoin`) are useful enough to have them in the release.
But I doubt there are many _automated_ deployments out there that obtain the release binaries and run the tests. That's the only thing this PR would break.
🚀 fanquake merged a pull request: "feature_taproot: sample tx version border values more"
(https://github.com/bitcoin/bitcoin/pull/32841)
(https://github.com/bitcoin/bitcoin/pull/32841)
🤔 hebasto reviewed a pull request: "depends: fix libevent `_WIN32_WINNT` usage"
(https://github.com/bitcoin/bitcoin/pull/32837#pullrequestreview-2978694585)
My Guix build:
```
aarch64
6a14c0d7863fcf63ad31f7327ba8bb6c05a8f187b23caf63031e4b383dafef6b guix-build-524dd98cb1cf/output/aarch64-linux-gnu/SHA256SUMS.part
fcb06e922b7a4e00f51f0517fbfbfa45fc0a9bb015f7f0dcead7e653c3fbfc93 guix-build-524dd98cb1cf/output/aarch64-linux-gnu/bitcoin-524dd98cb1cf-aarch64-linux-gnu-debug.tar.gz
c8f14621286d373d61490c0fbf07529c900cd88145c0d17fe6665220330f0fdc guix-build-524dd98cb1cf/output/aarch64-linux-gnu/bitcoin-524dd98cb1cf-aarch64-linux-gnu.tar.gz
9f65d8ac
...
(https://github.com/bitcoin/bitcoin/pull/32837#pullrequestreview-2978694585)
My Guix build:
```
aarch64
6a14c0d7863fcf63ad31f7327ba8bb6c05a8f187b23caf63031e4b383dafef6b guix-build-524dd98cb1cf/output/aarch64-linux-gnu/SHA256SUMS.part
fcb06e922b7a4e00f51f0517fbfbfa45fc0a9bb015f7f0dcead7e653c3fbfc93 guix-build-524dd98cb1cf/output/aarch64-linux-gnu/bitcoin-524dd98cb1cf-aarch64-linux-gnu-debug.tar.gz
c8f14621286d373d61490c0fbf07529c900cd88145c0d17fe6665220330f0fdc guix-build-524dd98cb1cf/output/aarch64-linux-gnu/bitcoin-524dd98cb1cf-aarch64-linux-gnu.tar.gz
9f65d8ac
...
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-3027512988)
Thank you for comments @achow101 and @maflcko!
I've applied both suggestions - making sure the benchmark is closer to the original, explained it in the commit message as well. I have re-measured each commit accordingly with the updated benchmarks - see the commit messages and PR description.
I have also reworked the renames to use simple scripted diffs for the renames (it even revealed a `xor_key` to `obfuscation` rename I missed and in one of the commit messages was still using the `XOR`
...
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-3027512988)
Thank you for comments @achow101 and @maflcko!
I've applied both suggestions - making sure the benchmark is closer to the original, explained it in the commit message as well. I have re-measured each commit accordingly with the updated benchmarks - see the commit messages and PR description.
I have also reworked the renames to use simple scripted diffs for the renames (it even revealed a `xor_key` to `obfuscation` rename I missed and in one of the commit messages was still using the `XOR`
...
💬 ismaelsadeeq commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2179737580)
nit licence update
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2179737580)
nit licence update
💬 ismaelsadeeq commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2179741353)
nit: since we are changing the datatype, also change the name to virtual_bytes?
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2179741353)
nit: since we are changing the datatype, also change the name to virtual_bytes?
🤔 ismaelsadeeq reviewed a pull request: "refactor: CFeeRate encapsulates FeeFrac internally"
(https://github.com/bitcoin/bitcoin/pull/32750#pullrequestreview-2978665608)
Code review 0aaeba9fe79df5ed51b3802056f93c0e7857a9fe
I think you should document the current behavior in `CFeeRate` docstring
1. Passing any virtual bytes less than or equal to 0 to the constructor will result in 0 fee rate per 0 size.
2. Passing negative virtual bytes to `GetFee` will return -1 as the fee.
(https://github.com/bitcoin/bitcoin/pull/32750#pullrequestreview-2978665608)
Code review 0aaeba9fe79df5ed51b3802056f93c0e7857a9fe
I think you should document the current behavior in `CFeeRate` docstring
1. Passing any virtual bytes less than or equal to 0 to the constructor will result in 0 fee rate per 0 size.
2. Passing negative virtual bytes to `GetFee` will return -1 as the fee.
💬 ismaelsadeeq commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2179806415)
why the assume, is it not better to just return the -1? and add a test for it?
Also it was added in first commit and removed here, the two commits should be squashed.
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2179806415)
why the assume, is it not better to just return the -1? and add a test for it?
Also it was added in first commit and removed here, the two commits should be squashed.
💬 ismaelsadeeq commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2179800839)
you can remove the type conversion, I think it is no-op because div is implicitly `int32_t`
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2179800839)
you can remove the type conversion, I think it is no-op because div is implicitly `int32_t`
💬 ismaelsadeeq commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2179747303)
nit: I think it should be clear that when using this constructor we also have 0 virtual bytes, I think it is possible to have 0 fee rate per n virtual bytes I think.
```suggestion
/** Fee rate of 0 satoshis per 0 vB */
```
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2179747303)
nit: I think it should be clear that when using this constructor we also have 0 virtual bytes, I think it is possible to have 0 fee rate per n virtual bytes I think.
```suggestion
/** Fee rate of 0 satoshis per 0 vB */
```
💬 ismaelsadeeq commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2179744660)
nit:
```suggestion
* Fee rate in satoshis per virtualbyte: CAmount / vB
* the feerate is represented internally as FeeFrac
```
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2179744660)
nit:
```suggestion
* Fee rate in satoshis per virtualbyte: CAmount / vB
* the feerate is represented internally as FeeFrac
```
💬 ismaelsadeeq commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2179823586)
Also same at `fee_rate` fuzz test.
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2179823586)
Also same at `fee_rate` fuzz test.
👍 hebasto approved a pull request: "depends: fix libevent `_WIN32_WINNT` usage"
(https://github.com/bitcoin/bitcoin/pull/32837#pullrequestreview-2978812923)
ACK 524dd98cb1cfa3a7917b94e13a91827842e7aba8, tested on macOS 15.5 (x64) using Homebrew's mingw-w64 13.0.0.
(https://github.com/bitcoin/bitcoin/pull/32837#pullrequestreview-2978812923)
ACK 524dd98cb1cfa3a7917b94e13a91827842e7aba8, tested on macOS 15.5 (x64) using Homebrew's mingw-w64 13.0.0.
✅ hebasto closed a pull request: "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows"
(https://github.com/bitcoin/bitcoin/pull/31158)
(https://github.com/bitcoin/bitcoin/pull/31158)
💬 l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2179856066)
I can confirm that I got the exact same result. I can also confirm that we didn't have any such block in the past decade.
Note that the biggest sigop per tx is 19971.
<img width="1000" alt="image" src="https://github.com/user-attachments/assets/b0a45fc6-98cb-4566-937f-e91234823505" />
sigops vs heights:
```
Found 2585 sigops in transaction 659135664894e50040830edb516a76f704fd2be409ecd8d1ea9916c002ab28a2 in block 228538
Found 4020 sigops in transaction bea1c2b87fee95a203c5b5d9f3e5d0f4
...
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2179856066)
I can confirm that I got the exact same result. I can also confirm that we didn't have any such block in the past decade.
Note that the biggest sigop per tx is 19971.
<img width="1000" alt="image" src="https://github.com/user-attachments/assets/b0a45fc6-98cb-4566-937f-e91234823505" />
sigops vs heights:
```
Found 2585 sigops in transaction 659135664894e50040830edb516a76f704fd2be409ecd8d1ea9916c002ab28a2 in block 228538
Found 4020 sigops in transaction bea1c2b87fee95a203c5b5d9f3e5d0f4
...
💬 l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2179860454)
> bumped into this limit, all either pathological or/and DoSy, all already non-standard by current rules
Question: is the reason against such massive coinjoins that they currently take too long to validate? Wouldn't forking based on that prohibit such transactions in a few decades when we could expect them not to be ddosy anymore?
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2179860454)
> bumped into this limit, all either pathological or/and DoSy, all already non-standard by current rules
Question: is the reason against such massive coinjoins that they currently take too long to validate? Wouldn't forking based on that prohibit such transactions in a few decades when we could expect them not to be ddosy anymore?
💬 fanquake commented on pull request "depends: fix libevent `_WIN32_WINNT` usage":
(https://github.com/bitcoin/bitcoin/pull/32837#issuecomment-3027621061)
Guix Build:
```bash
6a14c0d7863fcf63ad31f7327ba8bb6c05a8f187b23caf63031e4b383dafef6b guix-build-524dd98cb1cf/output/aarch64-linux-gnu/SHA256SUMS.part
fcb06e922b7a4e00f51f0517fbfbfa45fc0a9bb015f7f0dcead7e653c3fbfc93 guix-build-524dd98cb1cf/output/aarch64-linux-gnu/bitcoin-524dd98cb1cf-aarch64-linux-gnu-debug.tar.gz
c8f14621286d373d61490c0fbf07529c900cd88145c0d17fe6665220330f0fdc guix-build-524dd98cb1cf/output/aarch64-linux-gnu/bitcoin-524dd98cb1cf-aarch64-linux-gnu.tar.gz
9f65d8ace745d8f5
...
(https://github.com/bitcoin/bitcoin/pull/32837#issuecomment-3027621061)
Guix Build:
```bash
6a14c0d7863fcf63ad31f7327ba8bb6c05a8f187b23caf63031e4b383dafef6b guix-build-524dd98cb1cf/output/aarch64-linux-gnu/SHA256SUMS.part
fcb06e922b7a4e00f51f0517fbfbfa45fc0a9bb015f7f0dcead7e653c3fbfc93 guix-build-524dd98cb1cf/output/aarch64-linux-gnu/bitcoin-524dd98cb1cf-aarch64-linux-gnu-debug.tar.gz
c8f14621286d373d61490c0fbf07529c900cd88145c0d17fe6665220330f0fdc guix-build-524dd98cb1cf/output/aarch64-linux-gnu/bitcoin-524dd98cb1cf-aarch64-linux-gnu.tar.gz
9f65d8ace745d8f5
...