π martinus approved a pull request: "util: Use steady clock instead of system clock to measure durations"
(https://github.com/bitcoin/bitcoin/pull/27405#pullrequestreview-1415266523)
Code review ACK https://github.com/bitcoin/bitcoin/commit/fa83fb31619c19a1a30b4181486601a944941b16, also ran all tests. All usages of the steady_clock are just for duration measurements, so the change to a different epoch is ok.
(https://github.com/bitcoin/bitcoin/pull/27405#pullrequestreview-1415266523)
Code review ACK https://github.com/bitcoin/bitcoin/commit/fa83fb31619c19a1a30b4181486601a944941b16, also ran all tests. All usages of the steady_clock are just for duration measurements, so the change to a different epoch is ok.
π¬ pinheadmz commented on pull request "net: call getaddrinfo() in detachable thread to prevent stalling":
(https://github.com/bitcoin/bitcoin/pull/27557#issuecomment-1536631559)
...got thread sanitizer errors running locally on main when my local DNS is black-holed. Fixed that with a shared pointer.
(https://github.com/bitcoin/bitcoin/pull/27557#issuecomment-1536631559)
...got thread sanitizer errors running locally on main when my local DNS is black-holed. Fixed that with a shared pointer.
π¬ hebasto commented on pull request "build: Add CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/25797#issuecomment-1536643935)
Rebased on top of the bitcoin/bitcoin#27554.
The CI build is :green_circle:
(https://github.com/bitcoin/bitcoin/pull/25797#issuecomment-1536643935)
Rebased on top of the bitcoin/bitcoin#27554.
The CI build is :green_circle:
π€ glozow reviewed a pull request: "mempool / rpc: add getprioritisationmap, delete a mapDeltas entry when delta==0"
(https://github.com/bitcoin/bitcoin/pull/27501#pullrequestreview-1415387665)
Addressed review comments and moved it from rpc/mempool.cpp to rpc/mining.cpp.
> Probably miners would be thankful to not need multiply the result by 100000000 first in order to delete mapDelta entries (and it would also make the tests simpler π ), but happy to ACK either variant.
I agree! I've changed it to satoshis to be the same as prioritisetransaction.
(https://github.com/bitcoin/bitcoin/pull/27501#pullrequestreview-1415387665)
Addressed review comments and moved it from rpc/mempool.cpp to rpc/mining.cpp.
> Probably miners would be thankful to not need multiply the result by 100000000 first in order to delete mapDelta entries (and it would also make the tests simpler π ), but happy to ACK either variant.
I agree! I've changed it to satoshis to be the same as prioritisetransaction.
π¬ glozow commented on pull request "mempool / rpc: add getprioritisationmap, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1186458650)
Good point, changed to `getprioritisedtransactions`
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1186458650)
Good point, changed to `getprioritisedtransactions`
π¬ glozow commented on pull request "mempool / rpc: add getprioritisationmap, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1186459877)
Done
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1186459877)
Done
π¬ glozow commented on pull request "mempool / rpc: add getprioritisationmap, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1186461359)
Fixed
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1186461359)
Fixed
π¬ glozow commented on pull request "mempool / rpc: add getprioritisationmap, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1186460872)
Would it be API-breaky to change the result of prioritisetransaction?
I've edited the logs though, to show whether the tx is in mempool and what the new delta is. Btw I don't think `nTransactionsUpdated` starts out at 0 when this is called.
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1186460872)
Would it be API-breaky to change the result of prioritisetransaction?
I've edited the logs though, to show whether the tx is in mempool and what the new delta is. Btw I don't think `nTransactionsUpdated` starts out at 0 when this is called.
π¬ glozow commented on pull request "mempool / rpc: add getprioritisationmap, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1186461032)
Done
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1186461032)
Done
π¬ glozow commented on pull request "mempool / rpc: add getprioritisationmap, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1186459024)
Removed
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1186459024)
Removed
π¬ glozow commented on pull request "mempool / rpc: add getprioritisationmap, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1186459071)
Fixed
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1186459071)
Fixed
π¬ glozow commented on pull request "mempool / rpc: add getprioritisationmap, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1186459143)
Taken
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1186459143)
Taken
π¬ glozow commented on pull request "mempool / rpc: add getprioritisationmap, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1186461262)
Done, added a `std::optional<CAmount> modified_fee` to the `delta_info` struct
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1186461262)
Done, added a `std::optional<CAmount> modified_fee` to the `delta_info` struct
π¬ udiWertheimer commented on pull request "Allow accepting non-standard transactions on mainnet":
(https://github.com/bitcoin/bitcoin/pull/27578#issuecomment-1536733105)
> taproot was designed with specific goals in mind to allow people to do whatever they want, as long as they aren't causing systemic issues with relay/validation/miners.
>
> On the other hand, legacy script is full of DoS disasters(that we can't simply softfork out because literal _theft_), and we shouldn't just open the gates further for people to trash the network.
Admittedly I wasnβt aware of that distinction between taproot and pre-taproot scripts wrt DoS issues. Thatβs good news! If t
...
(https://github.com/bitcoin/bitcoin/pull/27578#issuecomment-1536733105)
> taproot was designed with specific goals in mind to allow people to do whatever they want, as long as they aren't causing systemic issues with relay/validation/miners.
>
> On the other hand, legacy script is full of DoS disasters(that we can't simply softfork out because literal _theft_), and we shouldn't just open the gates further for people to trash the network.
Admittedly I wasnβt aware of that distinction between taproot and pre-taproot scripts wrt DoS issues. Thatβs good news! If t
...
π¬ TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1536739318)
Rebased e056d6f758382d3418682095ab02f8d487aa641f -> 3b34ac7465919b968795063995f6610a73aa2d29 ([removeBlockstorageArgs_20](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_20) -> [removeBlockstorageArgs_21](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_21), [compare](https://github.com/TheCharlatan/bitcoin/compare/removeBlockstorageArgs_20..removeBlockstorageArgs_21))
* Fixed conflicts with #27570
* Added a commit to remove the params argument from the f
...
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1536739318)
Rebased e056d6f758382d3418682095ab02f8d487aa641f -> 3b34ac7465919b968795063995f6610a73aa2d29 ([removeBlockstorageArgs_20](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_20) -> [removeBlockstorageArgs_21](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_21), [compare](https://github.com/TheCharlatan/bitcoin/compare/removeBlockstorageArgs_20..removeBlockstorageArgs_21))
* Fixed conflicts with #27570
* Added a commit to remove the params argument from the f
...
π¬ TheCharlatan commented on pull request "refactor: Move chain constants to the util library":
(https://github.com/bitcoin/bitcoin/pull/27491#issuecomment-1536750187)
Rebased 7d361083d72a267de4af258e574219abdef0fc82 -> dd95e2a3353b5ded87d1d5408a51bf461f1f90b4 ([kernelChainType_2](https://github.com/TheCharlatan/bitcoin/tree/kernelChainType_2) -> [kernelChainType_3](https://github.com/TheCharlatan/bitcoin/tree/kernelChainType_3), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelChainType_2..kernelChainType_3))
* Fixed conflict with #27570
(https://github.com/bitcoin/bitcoin/pull/27491#issuecomment-1536750187)
Rebased 7d361083d72a267de4af258e574219abdef0fc82 -> dd95e2a3353b5ded87d1d5408a51bf461f1f90b4 ([kernelChainType_2](https://github.com/TheCharlatan/bitcoin/tree/kernelChainType_2) -> [kernelChainType_3](https://github.com/TheCharlatan/bitcoin/tree/kernelChainType_3), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelChainType_2..kernelChainType_3))
* Fixed conflict with #27570
π€ mzumsande reviewed a pull request: "fuzz: BIP 42, BIP 30, CVE-2018-17144"
(https://github.com/bitcoin/bitcoin/pull/17860#pullrequestreview-1415372134)
Tested ACK fa2d8b61f9343d350b67357a12f39b613c8ee8ad
I was also able to catch the reintroduced CVE-2018-17144 (with the previous commit fa18fe3976a0f99480ce42dc0c1df7143967bf4d, current one is still running).
I can't really see how this actually fuzzes BIP42. Wouldn't the fuzzer need to create millions of blocks to detect it, so if we'd introduce that bug, the fuzzer would never be able to find it in practice?
Finally, I also observed that the fuzzer gets slower with time and after a few
...
(https://github.com/bitcoin/bitcoin/pull/17860#pullrequestreview-1415372134)
Tested ACK fa2d8b61f9343d350b67357a12f39b613c8ee8ad
I was also able to catch the reintroduced CVE-2018-17144 (with the previous commit fa18fe3976a0f99480ce42dc0c1df7143967bf4d, current one is still running).
I can't really see how this actually fuzzes BIP42. Wouldn't the fuzzer need to create millions of blocks to detect it, so if we'd introduce that bug, the fuzzer would never be able to find it in practice?
Finally, I also observed that the fuzzer gets slower with time and after a few
...
π¬ mzumsande commented on pull request "fuzz: BIP 42, BIP 30, CVE-2018-17144":
(https://github.com/bitcoin/bitcoin/pull/17860#discussion_r1186451948)
why is this check inside the `was_valid` branch?
As far as I understand it, the second block with the duplicate coinbase could be valid (if the coinbase from block 1 was spent), or BIP-30 invalid (otherwise) - but this should hold regardless.
(https://github.com/bitcoin/bitcoin/pull/17860#discussion_r1186451948)
why is this check inside the `was_valid` branch?
As far as I understand it, the second block with the duplicate coinbase could be valid (if the coinbase from block 1 was spent), or BIP-30 invalid (otherwise) - but this should hold regardless.
π¬ instagibbs commented on pull request "Allow accepting non-standard transactions on mainnet":
(https://github.com/bitcoin/bitcoin/pull/27578#issuecomment-1536754509)
@udiWertheimer Assuming we could switch behavior ahead of time based on all inputs being taproot(it's a little more complicated in practice because other inputs might be ok too), I think the main drawback is optimization of the bin packing problem with larger items. Smaller items are easier to pick closer to optimally. This would fall under "keeping miners simple for decentralization" bucket instead of DoS.
(https://github.com/bitcoin/bitcoin/pull/27578#issuecomment-1536754509)
@udiWertheimer Assuming we could switch behavior ahead of time based on all inputs being taproot(it's a little more complicated in practice because other inputs might be ok too), I think the main drawback is optimization of the bin packing problem with larger items. Smaller items are easier to pick closer to optimally. This would fall under "keeping miners simple for decentralization" bucket instead of DoS.
π¬ berenddeboer commented on issue "[Linux] Add wayland support":
(https://github.com/bitcoin/bitcoin/issues/19950#issuecomment-1536761117)
> Is it the case that our bitcion-qt release binaries don't work on Wayland at all?
All I can say that on Ubuntu 23.04 + Sway (which uses Wayland), bitcoin-qt does not work, and crashes with:
```
> bitcoin-core.qt
Warning: Ignoring XDG_SESSION_TYPE=wayland on Gnome. Use QT_QPA_PLATFORM=wayland to run on Wayland anyway.
Fontconfig warning: FcPattern object weight does not accept value [0 205)
fish: Job 1, 'bitcoin-core.qt' terminated by signal SIGSEGV (Address boundary error)
```
D
...
(https://github.com/bitcoin/bitcoin/issues/19950#issuecomment-1536761117)
> Is it the case that our bitcion-qt release binaries don't work on Wayland at all?
All I can say that on Ubuntu 23.04 + Sway (which uses Wayland), bitcoin-qt does not work, and crashes with:
```
> bitcoin-core.qt
Warning: Ignoring XDG_SESSION_TYPE=wayland on Gnome. Use QT_QPA_PLATFORM=wayland to run on Wayland anyway.
Fontconfig warning: FcPattern object weight does not accept value [0 205)
fish: Job 1, 'bitcoin-core.qt' terminated by signal SIGSEGV (Address boundary error)
```
D
...