💬 maflcko commented on pull request "ci: add markdown link check job":
(https://github.com/bitcoin/bitcoin/pull/30034#issuecomment-2154279892)
`git ls-files` is also used in `./test/lint/lint-python.py`, for reference.
(https://github.com/bitcoin/bitcoin/pull/30034#issuecomment-2154279892)
`git ls-files` is also used in `./test/lint/lint-python.py`, for reference.
⚠️ maflcko opened an issue: "fuzz: timeout/oom in package_rbf"
(https://github.com/bitcoin/bitcoin/issues/30241)
I stumbled upon a fuzz seed that oom'ed in CI for `package_rbf`. Since the seed appears to be too big to paste, I provide the output of `$ base64 95a3b1b42d43ac7190f24ff116b2cd6515e7a566 > /tmp/base64-95a3b1b4-oom.txt`:
[base64-95a3b1b4-oom.txt](https://github.com/user-attachments/files/15685350/base64-95a3b1b4-oom.txt)
If the prior format is unsatisfactory, the problematic seed `95a3b1b42d43ac7190f24ff116b2cd6515e7a566` is also in this commit to qa-assets: https://github.com/b
...
(https://github.com/bitcoin/bitcoin/issues/30241)
I stumbled upon a fuzz seed that oom'ed in CI for `package_rbf`. Since the seed appears to be too big to paste, I provide the output of `$ base64 95a3b1b42d43ac7190f24ff116b2cd6515e7a566 > /tmp/base64-95a3b1b4-oom.txt`:
[base64-95a3b1b4-oom.txt](https://github.com/user-attachments/files/15685350/base64-95a3b1b4-oom.txt)
If the prior format is unsatisfactory, the problematic seed `95a3b1b42d43ac7190f24ff116b2cd6515e7a566` is also in this commit to qa-assets: https://github.com/b
...
💬 maflcko commented on issue "fuzz: timeout/oom in package_rbf":
(https://github.com/bitcoin/bitcoin/issues/30241#issuecomment-2154289292)
It spends a long time hashing the large transaction, which has a large witness. The runtime is NUM_ITERS (aka 10'000) times n (size of the transaction), because the hashing is done in the loop.
(https://github.com/bitcoin/bitcoin/issues/30241#issuecomment-2154289292)
It spends a long time hashing the large transaction, which has a large witness. The runtime is NUM_ITERS (aka 10'000) times n (size of the transaction), because the hashing is done in the loop.
💬 AngusP commented on pull request "test: Add Compact Block Encoding test `ReceiveWithExtraTransactions` covering non-empty `extra_txn`":
(https://github.com/bitcoin/bitcoin/pull/30237#issuecomment-2154290717)
> Not sure if that is worth addressing since it's so rare but perhaps simply adding a comment that the test might fail every 2^48 runs is good?
I think it's worth avoiding, I don't really like the notion of flaky tests maybe failing for a random reason, even if it is very improbable. I'd guess short-ID collisions would also affect some of the other existing tests in here so there's additional benefit in them not being flaky too.
(https://github.com/bitcoin/bitcoin/pull/30237#issuecomment-2154290717)
> Not sure if that is worth addressing since it's so rare but perhaps simply adding a comment that the test might fail every 2^48 runs is good?
I think it's worth avoiding, I don't really like the notion of flaky tests maybe failing for a random reason, even if it is very improbable. I'd guess short-ID collisions would also affect some of the other existing tests in here so there's additional benefit in them not being flaky too.
👍 dergoegge approved a pull request: "fuzz: add I2P harness"
(https://github.com/bitcoin/bitcoin/pull/30230#pullrequestreview-2103980713)
tACK 193c748e44f8647a056121fc9cbb9c2efbcbfc49
Thanks for picking this up!
I fuzzed the `i2p` harrness for a few hundred CPU hours and even without the dictionary it reaches almost everything. The dictionary will still help though, so adding it to qa-assets later would be good.
My only note: I think it would be worthwhile to point out in the PR description what changes you have made to the original harness and why they were necessary.
(https://github.com/bitcoin/bitcoin/pull/30230#pullrequestreview-2103980713)
tACK 193c748e44f8647a056121fc9cbb9c2efbcbfc49
Thanks for picking this up!
I fuzzed the `i2p` harrness for a few hundred CPU hours and even without the dictionary it reaches almost everything. The dictionary will still help though, so adding it to qa-assets later would be good.
My only note: I think it would be worthwhile to point out in the PR description what changes you have made to the original harness and why they were necessary.
💬 maflcko commented on issue "test: Intermittent issue in p2p_leak_tx.py in test_notfound_on_replaced_tx":
(https://github.com/bitcoin/bitcoin/issues/29621#issuecomment-2154400935)
https://drahtbot.space/temp_scratch/p2p_leak_tx_115_b.tar.zstd
```
test 2024-06-07T02:34:22.778000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 132, in main
self.run_test()
File "/ci_con
...
(https://github.com/bitcoin/bitcoin/issues/29621#issuecomment-2154400935)
https://drahtbot.space/temp_scratch/p2p_leak_tx_115_b.tar.zstd
```
test 2024-06-07T02:34:22.778000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 132, in main
self.run_test()
File "/ci_con
...
📝 hebasto opened a pull request: "ci: Native Windows CI job cleanup"
(https://github.com/bitcoin/bitcoin/pull/30242)
This PR:
1. Removes no longer needed workaround for GHA Windows images.
GHA Windows images previously had multiple VC Build Tools installed, which required specifying the `VCPKG_PLATFORM_TOOLSET_VERSION` explicitly to avoid linker errors. This issue has been resolved as per
https://github.com/actions/runner-images/issues/9701.
2. Switches all references to temporary files to relative ones for consistency and readability.
(https://github.com/bitcoin/bitcoin/pull/30242)
This PR:
1. Removes no longer needed workaround for GHA Windows images.
GHA Windows images previously had multiple VC Build Tools installed, which required specifying the `VCPKG_PLATFORM_TOOLSET_VERSION` explicitly to avoid linker errors. This issue has been resolved as per
https://github.com/actions/runner-images/issues/9701.
2. Switches all references to temporary files to relative ones for consistency and readability.
💬 AngusP commented on pull request "test: Added test coverage to listsinceblock rpc":
(https://github.com/bitcoin/bitcoin/pull/30195#discussion_r1630897082)
re. this comment https://github.com/bitcoin/bitcoin/pull/30195#discussion_r1630237777 and needing to use the full path again, I think it is that `rename(...)` returns a new `Path` instance, which is then the 'correct' one to use going forwards for further changes you want to happen on-disk (alas Python has no 'use of moved value' concept like Rust et al. that would've complained at you trying to do this).
I think the way to think of this is that a `Path` is just a *path*, not a file-handle, s
...
(https://github.com/bitcoin/bitcoin/pull/30195#discussion_r1630897082)
re. this comment https://github.com/bitcoin/bitcoin/pull/30195#discussion_r1630237777 and needing to use the full path again, I think it is that `rename(...)` returns a new `Path` instance, which is then the 'correct' one to use going forwards for further changes you want to happen on-disk (alas Python has no 'use of moved value' concept like Rust et al. that would've complained at you trying to do this).
I think the way to think of this is that a `Path` is just a *path*, not a file-handle, s
...
💬 AngusP commented on pull request "test: Added test coverage to listsinceblock rpc":
(https://github.com/bitcoin/bitcoin/pull/30195#discussion_r1630910069)
I've added https://github.com/bitcoin/bitcoin/pull/30195#discussion_r1630897082 with a suggestion
(https://github.com/bitcoin/bitcoin/pull/30195#discussion_r1630910069)
I've added https://github.com/bitcoin/bitcoin/pull/30195#discussion_r1630897082 with a suggestion
💬 AngusP commented on pull request "test: Added test coverage to listsinceblock rpc":
(https://github.com/bitcoin/bitcoin/pull/30195#discussion_r1630912317)
(Could even `blk_dat_moved.rename(blk_dat.name)` to avoid repeating the literal `"blk00000.dat"` but meh)
(https://github.com/bitcoin/bitcoin/pull/30195#discussion_r1630912317)
(Could even `blk_dat_moved.rename(blk_dat.name)` to avoid repeating the literal `"blk00000.dat"` but meh)
📝 hebasto converted_to_draft a pull request: "ci: Native Windows CI job cleanup"
(https://github.com/bitcoin/bitcoin/pull/30242)
This PR:
1. Removes no longer needed workaround for GHA Windows images.
GHA Windows images previously had multiple VC Build Tools installed, which required specifying the `VCPKG_PLATFORM_TOOLSET_VERSION` explicitly to avoid linker errors. This issue has been resolved as per
https://github.com/actions/runner-images/issues/9701.
2. Switches all references to temporary files to relative ones for consistency and readability.
(https://github.com/bitcoin/bitcoin/pull/30242)
This PR:
1. Removes no longer needed workaround for GHA Windows images.
GHA Windows images previously had multiple VC Build Tools installed, which required specifying the `VCPKG_PLATFORM_TOOLSET_VERSION` explicitly to avoid linker errors. This issue has been resolved as per
https://github.com/actions/runner-images/issues/9701.
2. Switches all references to temporary files to relative ones for consistency and readability.
👋 hebasto's pull request is ready for review: "ci: Native Windows CI job cleanup"
(https://github.com/bitcoin/bitcoin/pull/30242)
(https://github.com/bitcoin/bitcoin/pull/30242)
💬 hebasto commented on pull request "ci: Native Windows CI job cleanup":
(https://github.com/bitcoin/bitcoin/pull/30242#issuecomment-2154501642)
Friendly ping @m3dwards @sipsorcery ;)
(https://github.com/bitcoin/bitcoin/pull/30242#issuecomment-2154501642)
Friendly ping @m3dwards @sipsorcery ;)
💬 Sjors commented on pull request "Introduce Miner interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1630967421)
I'm now setting `uint256 tip{miner.getTipHash()};` at the top. For long polls it's updated after the wait. See d1be135e568605539fa3932da6e7c5032816825b.
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1630967421)
I'm now setting `uint256 tip{miner.getTipHash()};` at the top. For long polls it's updated after the wait. See d1be135e568605539fa3932da6e7c5032816825b.
💬 Sjors commented on pull request "Introduce Miner interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1630967487)
Renamed to `Mining`
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1630967487)
Renamed to `Mining`
💬 Sjors commented on pull request "Introduce Miner interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1630967663)
I replaced `getTip()` with `getTipHash()`. `Sv2TemplateProvider` doesn't need it, so for now I use it in a few places in `rpc/mining.cpp`, but then fall back to `chainman.m_blockman.LookupBlockIndex(miner.getTipHash())` where we need more info.
A future refactor could move more of `getblocktemplate` implementation details out of the RPC.
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1630967663)
I replaced `getTip()` with `getTipHash()`. `Sv2TemplateProvider` doesn't need it, so for now I use it in a few places in `rpc/mining.cpp`, but then fall back to `chainman.m_blockman.LookupBlockIndex(miner.getTipHash())` where we need more info.
A future refactor could move more of `getblocktemplate` implementation details out of the RPC.
💬 Sjors commented on pull request "Introduce Miner interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1630967783)
Thanks, replaced it with `get()`.
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1630967783)
Thanks, replaced it with `get()`.
💬 Sjors commented on pull request "Introduce Miner interface":
(https://github.com/bitcoin/bitcoin/pull/30200#issuecomment-2154504861)
@ryanofsky thanks for the review! I rebased and changed a few things to address comments, see inline.
Also added `IsInitialBlockDownload`, which was the last thing `Sv2TemplateProvider` still needed `ChainstateManager& m_chainman;` for.
I'll cherry-pick this into #29432 to see if we need anything more.
(https://github.com/bitcoin/bitcoin/pull/30200#issuecomment-2154504861)
@ryanofsky thanks for the review! I rebased and changed a few things to address comments, see inline.
Also added `IsInitialBlockDownload`, which was the last thing `Sv2TemplateProvider` still needed `ChainstateManager& m_chainman;` for.
I'll cherry-pick this into #29432 to see if we need anything more.
📝 Sjors converted_to_draft a pull request: "Introduce Mining interface"
(https://github.com/bitcoin/bitcoin/pull/30200)
Introduce a `Mining` interface for the `getblocktemplate` and `generateblock` RPC to use now, and for Stratum v2 to use later.
Suggested here: https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2108528652
The selection of methods added to the interface is mostly based on what the Template Provider in #29432 uses. It could be expanded further so that `rpc/mining.cpp` no longer needs `EnsureMemPool` and `EnsureChainman`.
This PR should be a pure refactor.
(https://github.com/bitcoin/bitcoin/pull/30200)
Introduce a `Mining` interface for the `getblocktemplate` and `generateblock` RPC to use now, and for Stratum v2 to use later.
Suggested here: https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2108528652
The selection of methods added to the interface is mostly based on what the Template Provider in #29432 uses. It could be expanded further so that `rpc/mining.cpp` no longer needs `EnsureMemPool` and `EnsureChainman`.
This PR should be a pure refactor.
💬 glozow commented on pull request "test: Add Compact Block Encoding test `ReceiveWithExtraTransactions` covering non-empty `extra_txn`":
(https://github.com/bitcoin/bitcoin/pull/30237#issuecomment-2154575591)
Concept ACK
> > Not sure if that is worth addressing since it's so rare but perhaps simply adding a comment that the test might fail every 2^48 runs is good?
>
> I think it's worth avoiding, I don't really like the notion of flaky tests maybe failing for a random reason, even if it is very improbable. I'd guess short-ID collisions would also affect some of the other existing tests in here so there's additional benefit in them not being flaky too.
Not sure exactly how flaky it is and don
...
(https://github.com/bitcoin/bitcoin/pull/30237#issuecomment-2154575591)
Concept ACK
> > Not sure if that is worth addressing since it's so rare but perhaps simply adding a comment that the test might fail every 2^48 runs is good?
>
> I think it's worth avoiding, I don't really like the notion of flaky tests maybe failing for a random reason, even if it is very improbable. I'd guess short-ID collisions would also affect some of the other existing tests in here so there's additional benefit in them not being flaky too.
Not sure exactly how flaky it is and don
...