💬 kevkevinpal commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#issuecomment-1879749727)
PR description link is routes to 404 can you update to use this https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_multisig_descriptor_psbt.py instead?
(https://github.com/bitcoin/bitcoin/pull/29156#issuecomment-1879749727)
PR description link is routes to 404 can you update to use this https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_multisig_descriptor_psbt.py instead?
💬 wizkid057 commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1879750836)
> Since CVE ID is used to validate this as a vulnerability
It's not, really. In fact, I didn't even mention "CVE" once in [my detailed comment above](https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1879268385).
This is clearly a bug, clearly a vulnerability in the software, clearly unintended behavior, and clearly being actively exploited in the wild. That _more_ than qualifies this particular issue as a valid CVE, but absolutely no one credible is doing the reverse of using
...
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1879750836)
> Since CVE ID is used to validate this as a vulnerability
It's not, really. In fact, I didn't even mention "CVE" once in [my detailed comment above](https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1879268385).
This is clearly a bug, clearly a vulnerability in the software, clearly unintended behavior, and clearly being actively exploited in the wild. That _more_ than qualifies this particular issue as a valid CVE, but absolutely no one credible is doing the reverse of using
...
💬 hebasto commented on pull request "build: remove `--enable-lto`":
(https://github.com/bitcoin/bitcoin/pull/29185#issuecomment-1879764210)
Tested 2d1b1c7daeeada3f737e62ceb2db7484cde5ff4e on Ubuntu 22.04:
```
$ ./configure CXXFLAGS="-g -O2 -flto=auto" SECP_CFLAGS="-flto=auto"
$ make -C src bitcoind
```
```
$ make -C depends CXXFLAGS="-O2 -flto=auto" LTO=1
$ ./configure CONFIG_SITE=$PWD/depends/x86_64-pc-linux-gnu/share/config.site SECP_CFLAGS="-flto=auto"
$ make
```
> Ah, I guess this would disable O2, see https://github.com/bitcoin/bitcoin/pull/28071/files. So I guess it would have to be appended to `CXX` and possib
...
(https://github.com/bitcoin/bitcoin/pull/29185#issuecomment-1879764210)
Tested 2d1b1c7daeeada3f737e62ceb2db7484cde5ff4e on Ubuntu 22.04:
```
$ ./configure CXXFLAGS="-g -O2 -flto=auto" SECP_CFLAGS="-flto=auto"
$ make -C src bitcoind
```
```
$ make -C depends CXXFLAGS="-O2 -flto=auto" LTO=1
$ ./configure CONFIG_SITE=$PWD/depends/x86_64-pc-linux-gnu/share/config.site SECP_CFLAGS="-flto=auto"
$ make
```
> Ah, I guess this would disable O2, see https://github.com/bitcoin/bitcoin/pull/28071/files. So I guess it would have to be appended to `CXX` and possib
...
💬 kristapsk commented on pull request "rpc: add 'getnetmsgstats' RPC":
(https://github.com/bitcoin/bitcoin/pull/28926#issuecomment-1879765838)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/28926#issuecomment-1879765838)
Concept ACK
💬 josibake commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#issuecomment-1879780205)
`git range-diff 5892014...909c6bb`
Addresses review comments from @aureleoules and @achow101 , most notable being now clearing `tx.vout` and asserting that it is empty inside both `FundTransaction` functions. This ensures this is no longer used to pass outputs to `FundTransaction`, now and in future code.
(https://github.com/bitcoin/bitcoin/pull/28560#issuecomment-1879780205)
`git range-diff 5892014...909c6bb`
Addresses review comments from @aureleoules and @achow101 , most notable being now clearing `tx.vout` and asserting that it is empty inside both `FundTransaction` functions. This ensures this is no longer used to pass outputs to `FundTransaction`, now and in future code.
💬 L0laL33tz commented on pull request "test: randomize perturbed files excluding ldb":
(https://github.com/bitcoin/bitcoin/pull/29182#discussion_r1443842409)
I tested this the same way @fjahr tested the size of the ldb files in #28831 with
```
self.log.info(f"Perturbing file to ensure failure {target_file}, size: {os.path.getsize(target_file)}")
```
The tested blockfile should always have 16777216 bytes
(https://github.com/bitcoin/bitcoin/pull/29182#discussion_r1443842409)
I tested this the same way @fjahr tested the size of the ldb files in #28831 with
```
self.log.info(f"Perturbing file to ensure failure {target_file}, size: {os.path.getsize(target_file)}")
```
The tested blockfile should always have 16777216 bytes
💬 dooglus commented on issue "new crash in v26.0":
(https://github.com/bitcoin/bitcoin/issues/29153#issuecomment-1879806191)
It happened again.
This node only connects to one other node. I run two nodes on the same machine. One connects to lots of peers, and this one that keeps crashing hosts my wallets, and only connects to the other node.
```
Thread 1 "bitcoin-qt-v26." received signal SIGSEGV, Segmentation fault.
0x00007ffff7a7c52d in ?? () from /lib/x86_64-linux-gnu/libQt5Core.so.5
(gdb) where
#0 0x00007ffff7a7c52d in ?? () from /lib/x86_64-linux-gnu/libQt5Core.so.5
#1 0x00007ffff7a82b57 in ?? () from
...
(https://github.com/bitcoin/bitcoin/issues/29153#issuecomment-1879806191)
It happened again.
This node only connects to one other node. I run two nodes on the same machine. One connects to lots of peers, and this one that keeps crashing hosts my wallets, and only connects to the other node.
```
Thread 1 "bitcoin-qt-v26." received signal SIGSEGV, Segmentation fault.
0x00007ffff7a7c52d in ?? () from /lib/x86_64-linux-gnu/libQt5Core.so.5
(gdb) where
#0 0x00007ffff7a7c52d in ?? () from /lib/x86_64-linux-gnu/libQt5Core.so.5
#1 0x00007ffff7a82b57 in ?? () from
...
💬 jonatack commented on pull request "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo":
(https://github.com/bitcoin/bitcoin/pull/29058#discussion_r1443852237)
> @jonatack do you have an opinion?
Am catching up with merged pulls and discussions; this one is high on my list when I've caught up.
(https://github.com/bitcoin/bitcoin/pull/29058#discussion_r1443852237)
> @jonatack do you have an opinion?
Am catching up with merged pulls and discussions; this one is high on my list when I've caught up.
💬 GregTonoski commented on issue "[Bug]: Blockspace price shouldn't be higher for a simple transaction (price discrimination against simple txs)":
(https://github.com/bitcoin/bitcoin/issues/29146#issuecomment-1879825003)
> Yes it takes more bytes to spend a transaction than to split a transaction because input consumption includes signatures (witness data) and outputs typically contain only P2SH which is compact. So the discount balances those input consumption bytes and output creation bytes so there is no longer a financial incentive to create dust. To be sure if you run out of coins your wallet will use change, but until then it will keep splitting coins. Say you have 100 1BTC lumps in your wallet for privacy
...
(https://github.com/bitcoin/bitcoin/issues/29146#issuecomment-1879825003)
> Yes it takes more bytes to spend a transaction than to split a transaction because input consumption includes signatures (witness data) and outputs typically contain only P2SH which is compact. So the discount balances those input consumption bytes and output creation bytes so there is no longer a financial incentive to create dust. To be sure if you run out of coins your wallet will use change, but until then it will keep splitting coins. Say you have 100 1BTC lumps in your wallet for privacy
...
💬 jonatack commented on pull request "sync: introduce a thread-safe generic container and use it to remove a bunch of "GlobalMutex"es":
(https://github.com/bitcoin/bitcoin/pull/25390#issuecomment-1879827022)
Will review the updates since my initial ACK https://github.com/bitcoin/bitcoin/pull/25390#pullrequestreview-1390434676.
(https://github.com/bitcoin/bitcoin/pull/25390#issuecomment-1879827022)
Will review the updates since my initial ACK https://github.com/bitcoin/bitcoin/pull/25390#pullrequestreview-1390434676.
💬 GregTonoski commented on issue "[Bug]: Blockspace price shouldn't be higher for a simple transaction (price discrimination against simple txs)":
(https://github.com/bitcoin/bitcoin/issues/29146#issuecomment-1879828846)
https://delvingbitcoin.org/t/bug-spammers-get-bitcoin-blockspace-at-discounted-price-lets-fix-it/327/4?u=gregtonoski
(https://github.com/bitcoin/bitcoin/issues/29146#issuecomment-1879828846)
https://delvingbitcoin.org/t/bug-spammers-get-bitcoin-blockspace-at-discounted-price-lets-fix-it/327/4?u=gregtonoski
💬 jonatack commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#issuecomment-1879829758)
> I like the simplified interface, having to write something like `LogPrintLevel(BCLog::NET, BCLog::Level::Error, ...)` for each log would be annoying
The idea behind the severity level logging was to have a single macro like `Log(category, level)` with a consistent, simple interface. See https://github.com/bitcoin/bitcoin/pull/28318#pullrequestreview-1605951639, above.
(https://github.com/bitcoin/bitcoin/pull/28318#issuecomment-1879829758)
> I like the simplified interface, having to write something like `LogPrintLevel(BCLog::NET, BCLog::Level::Error, ...)` for each log would be annoying
The idea behind the severity level logging was to have a single macro like `Log(category, level)` with a consistent, simple interface. See https://github.com/bitcoin/bitcoin/pull/28318#pullrequestreview-1605951639, above.
👋 hebasto's pull request is ready for review: "build: Pass sanitize flags to instrument `libsecp256k1` code"
(https://github.com/bitcoin/bitcoin/pull/28875)
(https://github.com/bitcoin/bitcoin/pull/28875)
🤔 jonatack reviewed a pull request: "RFC: Deprecate libconsensus"
(https://github.com/bitcoin/bitcoin/pull/29189#pullrequestreview-1807616432)
Light ACK a6745f8be306a0aeeb7fb070297ba503321891d0 pending the actions above
Could probably squash. IIUC closes https://github.com/bitcoin/bitcoin/pull/29123.
(https://github.com/bitcoin/bitcoin/pull/29189#pullrequestreview-1807616432)
Light ACK a6745f8be306a0aeeb7fb070297ba503321891d0 pending the actions above
Could probably squash. IIUC closes https://github.com/bitcoin/bitcoin/pull/29123.
💬 jonatack commented on pull request "validation: improve performance of CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/28339#issuecomment-1879863374)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/28339#issuecomment-1879863374)
Concept ACK
💬 theStack commented on pull request "contrib: add tool to convert compact-serialized UTXO set to SQLite database":
(https://github.com/bitcoin/bitcoin/pull/27432#discussion_r1443869992)
Done.
(https://github.com/bitcoin/bitcoin/pull/27432#discussion_r1443869992)
Done.
💬 theStack commented on pull request "contrib: add tool to convert compact-serialized UTXO set to SQLite database":
(https://github.com/bitcoin/bitcoin/pull/27432#discussion_r1443870383)
Done. Also, moved this condition down to after the written coin statistic output, as it is still useful to see in the EOF-not-reached-yet case.
(https://github.com/bitcoin/bitcoin/pull/27432#discussion_r1443870383)
Done. Also, moved this condition down to after the written coin statistic output, as it is still useful to see in the EOF-not-reached-yet case.
💬 theStack commented on pull request "contrib: add tool to convert compact-serialized UTXO set to SQLite database":
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-1879866925)
> Could mark as draft while CI is red?
Sorry for the extra-late reply, missed this message and the CI fail. Rebased on master and resolved the silent merge conflict (caused by the module move `test_framework.muhash` -> `test_framework.crypto.muhash` in #28374). Also fixed the Windows CI issue by closing the sqlite connections properly with explicit `con.close()` calls (see e.g. https://github.com/bitcoin/bitcoin/pull/28204). CI is green now.
>> What is the rationale for encoding as text ra
...
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-1879866925)
> Could mark as draft while CI is red?
Sorry for the extra-late reply, missed this message and the CI fail. Rebased on master and resolved the silent merge conflict (caused by the module move `test_framework.muhash` -> `test_framework.crypto.muhash` in #28374). Also fixed the Windows CI issue by closing the sqlite connections properly with explicit `con.close()` calls (see e.g. https://github.com/bitcoin/bitcoin/pull/28204). CI is green now.
>> What is the rationale for encoding as text ra
...
💬 hebasto commented on pull request "build: Pass sanitize flags to instrument `libsecp256k1` code":
(https://github.com/bitcoin/bitcoin/pull/28875#issuecomment-1879869514)
The CI has been fixed and this PR is ready for review now.
Friendly ping @dergoegge :)
(https://github.com/bitcoin/bitcoin/pull/28875#issuecomment-1879869514)
The CI has been fixed and this PR is ready for review now.
Friendly ping @dergoegge :)
🤔 theStack reviewed a pull request: "test: Treat msg_version.relay as unsigned, Remove `struct` packing in messages.py"
(https://github.com/bitcoin/bitcoin/pull/29067#pullrequestreview-1807648209)
Concept ACK
I agree with the outlined drawbacks of `struct.{un,}pack` (especially the "format string consists of characters that may be confusing and may need to be looked up in the documentation" part) and that using `int.{from,to}_bytes` should be used instead.
Small side-note unrelated to this PR:
For (de)serializing single bytes I usually prefer `bytes([val])` over `val.to_bytes(1, "little")` (and `bs[0]` over `int.from_bytes(bs, "little")`), as it feels a bit weird having to specify
...
(https://github.com/bitcoin/bitcoin/pull/29067#pullrequestreview-1807648209)
Concept ACK
I agree with the outlined drawbacks of `struct.{un,}pack` (especially the "format string consists of characters that may be confusing and may need to be looked up in the documentation" part) and that using `int.{from,to}_bytes` should be used instead.
Small side-note unrelated to this PR:
For (de)serializing single bytes I usually prefer `bytes([val])` over `val.to_bytes(1, "little")` (and `bs[0]` over `int.from_bytes(bs, "little")`), as it feels a bit weird having to specify
...