👍 stickies-v approved a pull request: "Bump python minimum supported version to 3.10"
(https://github.com/bitcoin/bitcoin/pull/30527#pullrequestreview-2369902240)
ACK fa1b139d17d04cb23bdfb1dd9c2abcdad4bdcd27
> For reference:
> ...
> * https://packages.ubuntu.com/jammy/python3
Note: focal is [supported until April 2025](https://ubuntu.com/about/release-cycle) and ships with [`3.8`](https://packages.ubuntu.com/focal/python3), so since that's already unsupported as per our current `3.9` requirements that doesn't really change anything.
(https://github.com/bitcoin/bitcoin/pull/30527#pullrequestreview-2369902240)
ACK fa1b139d17d04cb23bdfb1dd9c2abcdad4bdcd27
> For reference:
> ...
> * https://packages.ubuntu.com/jammy/python3
Note: focal is [supported until April 2025](https://ubuntu.com/about/release-cycle) and ships with [`3.8`](https://packages.ubuntu.com/focal/python3), so since that's already unsupported as per our current `3.9` requirements that doesn't really change anything.
💬 glozow commented on pull request "doc: add dustThreshold explain of P2SH & P2TR":
(https://github.com/bitcoin/bitcoin/pull/30023#discussion_r1801509062)
I somewhat agree that if the code does something slightly different from what one could expect, it may be worth adding a comment - it would depend on the situation. Verbose discussions of engineering tradeoffs usually belong in discussion forums, docs, and bips, not function descriptions.
What we have currently seems fine to me: "Note this computation is for spending a Segwit v0... For Segwit v1... this computation was kept. See discussion... for more details". It is in a suitable location, i
...
(https://github.com/bitcoin/bitcoin/pull/30023#discussion_r1801509062)
I somewhat agree that if the code does something slightly different from what one could expect, it may be worth adding a comment - it would depend on the situation. Verbose discussions of engineering tradeoffs usually belong in discussion forums, docs, and bips, not function descriptions.
What we have currently seems fine to me: "Note this computation is for spending a Segwit v0... For Segwit v1... this computation was kept. See discussion... for more details". It is in a suitable location, i
...
💬 jasonandjay commented on pull request "doc: add dustThreshold explain of P2SH & P2TR":
(https://github.com/bitcoin/bitcoin/pull/30023#discussion_r1801516249)
OK, I agree with you. It may be more appropriate to put it in discuss.
I will close this issue. Thank you for your review.
(https://github.com/bitcoin/bitcoin/pull/30023#discussion_r1801516249)
OK, I agree with you. It may be more appropriate to put it in discuss.
I will close this issue. Thank you for your review.
✅ jasonandjay closed a pull request: "doc: add dustThreshold explain of P2SH & P2TR"
(https://github.com/bitcoin/bitcoin/pull/30023)
(https://github.com/bitcoin/bitcoin/pull/30023)
🤔 TheCharlatan reviewed a pull request: "Safegcd-based modular inverses in MuHash3072"
(https://github.com/bitcoin/bitcoin/pull/21590#pullrequestreview-2369959877)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/21590#pullrequestreview-2369959877)
Concept ACK
💬 panicfarm commented on issue "LevelDB read failure: Corruption: block checksum mismatch":
(https://github.com/bitcoin/bitcoin/issues/30159#issuecomment-2414498815)
I wanted to weigh in here. Tried to sync a 27.1 node on a KVM with Ubuntu 24.04.01/xfs. During the initial sync I got mysterious crashes, apparently due to `.dat` files or LevelDB corruption. I unmounted the volume, but `xfs_repair` did not find any filesystem errors. Downgraded the node to `25.0`, segfaults still persisted. I also checked the KVM's RAM with memtest86+, no errors. In the kernel log there were some xfs warnings however.
After this, I reinstalled 24.04.01 on the same KVM with
...
(https://github.com/bitcoin/bitcoin/issues/30159#issuecomment-2414498815)
I wanted to weigh in here. Tried to sync a 27.1 node on a KVM with Ubuntu 24.04.01/xfs. During the initial sync I got mysterious crashes, apparently due to `.dat` files or LevelDB corruption. I unmounted the volume, but `xfs_repair` did not find any filesystem errors. Downgraded the node to `25.0`, segfaults still persisted. I also checked the KVM's RAM with memtest86+, no errors. In the kernel log there were some xfs warnings however.
After this, I reinstalled 24.04.01 on the same KVM with
...
✅ achow101 closed a pull request: "set `DEFAULT_PERMIT_BAREMULTISIG` to false"
(https://github.com/bitcoin/bitcoin/pull/28217)
(https://github.com/bitcoin/bitcoin/pull/28217)
💬 achow101 commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-2414516227)
Closing as this lacks conceptual support and is still obviously controversial.
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-2414516227)
Closing as this lacks conceptual support and is still obviously controversial.
✅ achow101 closed an issue: "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)"
(https://github.com/bitcoin/bitcoin/issues/29187)
(https://github.com/bitcoin/bitcoin/issues/29187)
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1801611171)
Good point - I feel like we actually shouldn't be adding the `Assume(IsInvalid())` here then. We don't have the `MempoolAcceptResult` here (and we shouldn't do that imo as txdownloadman.h would have a dependency on validation.h)
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1801611171)
Good point - I feel like we actually shouldn't be adding the `Assume(IsInvalid())` here then. We don't have the `MempoolAcceptResult` here (and we shouldn't do that imo as txdownloadman.h would have a dependency on validation.h)
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1801611396)
Ooh good catch.
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1801611396)
Ooh good catch.
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1801661898)
Thanks for noticing, very subtle! Added a commit to make this change explicit, though it is overwritten immediately afterwards.
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1801661898)
Thanks for noticing, very subtle! Added a commit to make this change explicit, though it is overwritten immediately afterwards.
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1801662930)
Added a comment that this specifically refers to peers in the map.
I also added a doxygen comment to the `TxDownloadManager` definition saying that this class isn't thread-safe and needs to be externally synced.
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1801662930)
Added a comment that this specifically refers to peers in the map.
I also added a doxygen comment to the `TxDownloadManager` definition saying that this class isn't thread-safe and needs to be externally synced.
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2414663136)
Resolved https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2391859626 by adding this:
> don't return package if child is in rejects filter(seems most robust?)
I like this suggestion as well:
> Call TxOrphanage::EraseTx using both txid and wtxid in this case (witness-free tx can never be valid)
I've dropped the one_honest_peer fuzzer and plan to open it as a separate PR, because it's quite a large portion of the new code and imo underripe compared to the rest of the changes.
...
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2414663136)
Resolved https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2391859626 by adding this:
> don't return package if child is in rejects filter(seems most robust?)
I like this suggestion as well:
> Call TxOrphanage::EraseTx using both txid and wtxid in this case (witness-free tx can never be valid)
I've dropped the one_honest_peer fuzzer and plan to open it as a separate PR, because it's quite a large portion of the new code and imo underripe compared to the rest of the changes.
...
💬 1440000bytes commented on pull request "wallet: Deniability API (Unilateral Transaction Meta-Privacy)":
(https://github.com/bitcoin/bitcoin/pull/27792#issuecomment-2414671643)
Its true there was a lack of interest to review this pull request and its not the best tool for privacy. However, a worse way to do the same thing was used in Samourai earlier and still available in Ashigaru. Ricochet fee is 0.001 BTC for the default 4 hops, with an additional fee for each extra hop beyond 4.

http://ashicodepbnpvslzsl2bz7l2pwrjvajgumgac423pp3y2deprbnzz7id.onion/Ashigaru/Ashigaru-Mob
...
(https://github.com/bitcoin/bitcoin/pull/27792#issuecomment-2414671643)
Its true there was a lack of interest to review this pull request and its not the best tool for privacy. However, a worse way to do the same thing was used in Samourai earlier and still available in Ashigaru. Ricochet fee is 0.001 BTC for the default 4 hops, with an additional fee for each extra hop beyond 4.

http://ashicodepbnpvslzsl2bz7l2pwrjvajgumgac423pp3y2deprbnzz7id.onion/Ashigaru/Ashigaru-Mob
...
⚠️ D33r-Gee opened an issue: "cpp: exposing AssumeUTXO functionality to the GUI (QML) via the Node interface"
(https://github.com/bitcoin/bitcoin/issues/31094)
### Issues, reports or feature requests related to the GUI should be opened directly on the GUI repo
- [X] I still think this issue should be opened here
### Report
To facilitate UTXO snapshot loading through the GUI, specifically for the QML implementation, we need to extend the Node interface in [src/interfaces/node.h](https://github.com/bitcoin/bitcoin/blob/master/src/interfaces/node.h) and provide a corresponding implementation in [src/node/interfaces.cpp](https://github.com/bitcoin/bitco
...
(https://github.com/bitcoin/bitcoin/issues/31094)
### Issues, reports or feature requests related to the GUI should be opened directly on the GUI repo
- [X] I still think this issue should be opened here
### Report
To facilitate UTXO snapshot loading through the GUI, specifically for the QML implementation, we need to extend the Node interface in [src/interfaces/node.h](https://github.com/bitcoin/bitcoin/blob/master/src/interfaces/node.h) and provide a corresponding implementation in [src/node/interfaces.cpp](https://github.com/bitcoin/bitco
...
💬 adamandrews1 commented on pull request "fees: document non-monotonic estimation edge case":
(https://github.com/bitcoin/bitcoin/pull/31080#discussion_r1801864730)
Nit: The lead is a bit buried here. I would plainly state "Monotonically increasing estimates are not guaranteed in certain data-sparse scenarios. In particular, (fill in the rest of your comment)"
(https://github.com/bitcoin/bitcoin/pull/31080#discussion_r1801864730)
Nit: The lead is a bit buried here. I would plainly state "Monotonically increasing estimates are not guaranteed in certain data-sparse scenarios. In particular, (fill in the rest of your comment)"
💬 jarolrod commented on issue "cpp: exposing AssumeUTXO functionality to the GUI (QML) via the Node interface":
(https://github.com/bitcoin/bitcoin/issues/31094#issuecomment-2415197540)
I don't think this needs an issue opened up here yet, the draft implementation linked isn't ready for review.
It would be unclear who the user would be now in relation to this repo with the Qt Widgets GUI not having a design to represent AssumeUTXO states.
Issues related to the QML GUI should be contained in that [repo](https://github.com/bitcoin-core/gui-qml), until it's clearly relevant here.
(https://github.com/bitcoin/bitcoin/issues/31094#issuecomment-2415197540)
I don't think this needs an issue opened up here yet, the draft implementation linked isn't ready for review.
It would be unclear who the user would be now in relation to this repo with the Qt Widgets GUI not having a design to represent AssumeUTXO states.
Issues related to the QML GUI should be contained in that [repo](https://github.com/bitcoin-core/gui-qml), until it's clearly relevant here.
💬 furszy commented on pull request "rpc: add `revelant_blocks` to `scanblocks status`":
(https://github.com/bitcoin/bitcoin/pull/30713#issuecomment-2415235999)
> Perhaps @furszy, that https://github.com/bitcoin/bitcoin/pull/26780 on a https://github.com/bitcoin/bitcoin/pull/23549#pullrequestreview-1105712566 of the rpc command, has an idea if it's possible (and if worth it).
Not sure if it worth it but could craft an unit test that calls to the RPC command. Subclassing and setting the block manager class so the file opening method "stops" at a certain point. It would mimic the delay.
(https://github.com/bitcoin/bitcoin/pull/30713#issuecomment-2415235999)
> Perhaps @furszy, that https://github.com/bitcoin/bitcoin/pull/26780 on a https://github.com/bitcoin/bitcoin/pull/23549#pullrequestreview-1105712566 of the rpc command, has an idea if it's possible (and if worth it).
Not sure if it worth it but could craft an unit test that calls to the RPC command. Subclassing and setting the block manager class so the file opening method "stops" at a certain point. It would mimic the delay.
💬 willcl-ark commented on pull request "fees: document non-monotonic estimation edge case":
(https://github.com/bitcoin/bitcoin/pull/31080#discussion_r1802066494)
Thanks, I'm happy to take this suggestion. What do you think about:
* Note: Monotonically increasing estimates are not guaranteed in certain
* data-sparse scenarios. In particular, if estimateCombinedFee returns no
* valid data (-1) for some estimates for a lower target, but does return
* valid data for a higher target, the estimate can theoretically return a
* higher value for higher targets.
(https://github.com/bitcoin/bitcoin/pull/31080#discussion_r1802066494)
Thanks, I'm happy to take this suggestion. What do you think about:
* Note: Monotonically increasing estimates are not guaranteed in certain
* data-sparse scenarios. In particular, if estimateCombinedFee returns no
* valid data (-1) for some estimates for a lower target, but does return
* valid data for a higher target, the estimate can theoretically return a
* higher value for higher targets.