💬 darosior commented on pull request "validation: detect witness stripping early on":
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2245449707)
I think this would be a bit confusing because it would need to return `true` for undefined program types, whereas we don't know yes if this type will require a witness or not (presumably yes, but hey). I also think it's brief enough to not warrant its own function. Happy to change it, but it seems cleaner this way.
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2245449707)
I think this would be a bit confusing because it would need to return `true` for undefined program types, whereas we don't know yes if this type will require a witness or not (presumably yes, but hey). I also think it's brief enough to not warrant its own function. Happy to change it, but it seems cleaner this way.
💬 darosior commented on pull request "validation: detect witness stripping early on":
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2245453045)
self nit: if i retouch, 0e22a1401c7edee8946f404dd0deb59a94231340's message is missing a word at the end of the second sentence.
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2245453045)
self nit: if i retouch, 0e22a1401c7edee8946f404dd0deb59a94231340's message is missing a word at the end of the second sentence.
💬 darosior commented on pull request "Bump SCRIPT_VERIFY flags to 64 bit":
(https://github.com/bitcoin/bitcoin/pull/32998#issuecomment-3140066988)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/32998#issuecomment-3140066988)
Concept ACK.
🤔 furszy reviewed a pull request: "index: fix wrong assert of current_tip == m_best_block_index"
(https://github.com/bitcoin/bitcoin/pull/32878#pullrequestreview-3075577814)
> Hi @furszy , sorry for the delay, I've picked your commit of the test, which I missed earlier.
No problem. This needs a rebase.
(https://github.com/bitcoin/bitcoin/pull/32878#pullrequestreview-3075577814)
> Hi @furszy , sorry for the delay, I've picked your commit of the test, which I missed earlier.
No problem. This needs a rebase.
✅ fanquake closed an issue: "tsan: drop `DEBUG_LOCKORDER` macro from TSAN job?"
(https://github.com/bitcoin/bitcoin/issues/33087)
(https://github.com/bitcoin/bitcoin/issues/33087)
🚀 fanquake merged a pull request: "ci: allow for any libc++ intrumentation & use it for TSAN"
(https://github.com/bitcoin/bitcoin/pull/33099)
(https://github.com/bitcoin/bitcoin/pull/33099)
💬 willcl-ark commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3140218000)
Pushed to fix the merge conflict, and addressed nits
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3140218000)
Pushed to fix the merge conflict, and addressed nits
💬 willcl-ark commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2245577820)
fixed in 7a33a8bea13 ci: add Cirrus cache host
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2245577820)
fixed in 7a33a8bea13 ci: add Cirrus cache host
💬 willcl-ark commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2245578296)
Fixed in 1587a7baa96 ci: dynamically match makejobs with cores
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2245578296)
Fixed in 1587a7baa96 ci: dynamically match makejobs with cores
💬 fanquake commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2245584929)
Should be no gui after #33099
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2245584929)
Should be no gui after #33099
💬 nervana21 commented on pull request "validation: detect witness stripping early on":
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2245589620)
Small nit. This comment could be updated now that we're checking for both `nonstandard` and `stripped` witnesses
```suggestion
// Check for witness-related policy violations.
```
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2245589620)
Small nit. This comment could be updated now that we're checking for both `nonstandard` and `stripped` witnesses
```suggestion
// Check for witness-related policy violations.
```
💬 fanquake commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3140270122)
TSAN failure (https://github.com/bitcoin/bitcoin/actions/runs/16651908244/job/47126650397?pr=32989#step:8:6056):
```bash
2025-07-31T14:39:40.396402Z [test] [net.cpp:2419] [void CConnman::SetTryNewOutboundPeer(bool)] [net] setting try another outbound peer=false
2025-07-31T14:39:40.396438Z [test] [net.cpp:3192] [void==================
WARNING: ThreadSanitizer: data race (pid=11584)
Write of size 8 at 0x7234000016f0 by main thread:
#0 blockmanager_tests::blockmanager_readblock_hash_mi
...
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3140270122)
TSAN failure (https://github.com/bitcoin/bitcoin/actions/runs/16651908244/job/47126650397?pr=32989#step:8:6056):
```bash
2025-07-31T14:39:40.396402Z [test] [net.cpp:2419] [void CConnman::SetTryNewOutboundPeer(bool)] [net] setting try another outbound peer=false
2025-07-31T14:39:40.396438Z [test] [net.cpp:3192] [void==================
WARNING: ThreadSanitizer: data race (pid=11584)
Write of size 8 at 0x7234000016f0 by main thread:
#0 blockmanager_tests::blockmanager_readblock_hash_mi
...
💬 1440000bytes commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3140353595)
Actually minrelaytxfee should be 10 sat/vbyte
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3140353595)
Actually minrelaytxfee should be 10 sat/vbyte
👍 pablomartin4btc approved a pull request: "test: Use the same mocktime when migrating in wallet_migration.py"
(https://github.com/bitcoin/bitcoin/pull/33104#pullrequestreview-3075915901)
ACK 95c11728f423e1c655439f9248a314f083ef68ef
Since now `migrate_and_get_rpc` allows to the `mocked_time` to be passed, as it was already validating the timestamp in the backup filename within the function, wouldn't make sense anymore to do the validation [again](https://github.com/bitcoin/bitcoin/pull/33104/files#r2244750342) outside the function.
(https://github.com/bitcoin/bitcoin/pull/33104#pullrequestreview-3075915901)
ACK 95c11728f423e1c655439f9248a314f083ef68ef
Since now `migrate_and_get_rpc` allows to the `mocked_time` to be passed, as it was already validating the timestamp in the backup filename within the function, wouldn't make sense anymore to do the validation [again](https://github.com/bitcoin/bitcoin/pull/33104/files#r2244750342) outside the function.
💬 Crypt-iQ commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2245743993)
Fixed in latest push. Only modification to https://github.com/stickies-v/bitcoin/commit/3d96ff75f161419654b14a7e9fd884e52aec26c4 was that I removed the `LogInstance().EnableCategory(BCLog::LogFlags::NONE);` line.
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2245743993)
Fixed in latest push. Only modification to https://github.com/stickies-v/bitcoin/commit/3d96ff75f161419654b14a7e9fd884e52aec26c4 was that I removed the `LogInstance().EnableCategory(BCLog::LogFlags::NONE);` line.
💬 Crypt-iQ commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#issuecomment-3140442210)
Latest push d434155db5f9c34b8ab3e19038d824d161b34195 -> f57496e:
- ensures the category mask does not leak across tests
- modifies `logging_filesize_rate_limit` to disable `BCLog::LogFlags::LOCK` if `DEBUG_LOCKCONTENTION` is defined
This gets rid of the test flake without hacking around `ReadDebugLogLines`.
(https://github.com/bitcoin/bitcoin/pull/33011#issuecomment-3140442210)
Latest push d434155db5f9c34b8ab3e19038d824d161b34195 -> f57496e:
- ensures the category mask does not leak across tests
- modifies `logging_filesize_rate_limit` to disable `BCLog::LogFlags::LOCK` if `DEBUG_LOCKCONTENTION` is defined
This gets rid of the test flake without hacking around `ReadDebugLogLines`.
💬 maflcko commented on pull request "ci: limit max stack size to 512 KiB":
(https://github.com/bitcoin/bitcoin/pull/33079#issuecomment-3140480780)
Though, it fails on GHA: https://github.com/maflcko/bitcoin-core-with-ci/actions/runs/16649916226/job/47119624805#step:8:30398
Without a way to reproduce interactively, it makes it hard to debug :(
(https://github.com/bitcoin/bitcoin/pull/33079#issuecomment-3140480780)
Though, it fails on GHA: https://github.com/maflcko/bitcoin-core-with-ci/actions/runs/16649916226/job/47119624805#step:8:30398
Without a way to reproduce interactively, it makes it hard to debug :(
💬 fanquake commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3140526097)
> TSAN failure
Didn't fail with a re-run: https://github.com/bitcoin/bitcoin/actions/runs/16651908244/job/47133023619.
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3140526097)
> TSAN failure
Didn't fail with a re-run: https://github.com/bitcoin/bitcoin/actions/runs/16651908244/job/47133023619.
💬 stickies-v commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2245830190)
Yeah sorry that should've been `LogInstance().DisableCategory(BCLog::LogFlags::ALL);`. I still think that's worth keeping, so we can always assume `LogTest` starts without any categories (as is default)?
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2245830190)
Yeah sorry that should've been `LogInstance().DisableCategory(BCLog::LogFlags::ALL);`. I still think that's worth keeping, so we can always assume `LogTest` starts without any categories (as is default)?
💬 TheCharlatan commented on pull request "refactor: inline constant return values from `dbwrapper` write methods":
(https://github.com/bitcoin/bitcoin/pull/33042#issuecomment-3140574746)
Concept ACK
Implemented the same thing some time ago https://github.com/TheCharlatan/bitcoin/commit/de7e3a973ef5773714cd15ead499fbf734547cf1#diff-ac5fbeb5de6f28370bc348a579fc465fe7f7b91df0e0483c6edbf10273278c0d.
(https://github.com/bitcoin/bitcoin/pull/33042#issuecomment-3140574746)
Concept ACK
Implemented the same thing some time ago https://github.com/TheCharlatan/bitcoin/commit/de7e3a973ef5773714cd15ead499fbf734547cf1#diff-ac5fbeb5de6f28370bc348a579fc465fe7f7b91df0e0483c6edbf10273278c0d.