💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#issuecomment-2088994389)
@ryanofsky thanks so much, again, for your help. Especially in understanding the `std::move` semantics. I've reviewed each commit in your branch and force pushed it to mine with the only change being specifying json request version in `JSONRPCReplyObj()` calls in `bitcoin-cli.cpp`
(https://github.com/bitcoin/bitcoin/pull/27101#issuecomment-2088994389)
@ryanofsky thanks so much, again, for your help. Especially in understanding the `std::move` semantics. I've reviewed each commit in your branch and force pushed it to mine with the only change being specifying json request version in `JSONRPCReplyObj()` calls in `bitcoin-cli.cpp`
👍 TheCharlatan approved a pull request: "build: Assume HAVE_CONFIG_H, Add IWYU pragma keep to bitcoin-config.h includes"
(https://github.com/bitcoin/bitcoin/pull/29494#pullrequestreview-2034337579)
ACK fa09451f8e6799682d7e7c863f25334fd1c7dce3
(https://github.com/bitcoin/bitcoin/pull/29494#pullrequestreview-2034337579)
ACK fa09451f8e6799682d7e7c863f25334fd1c7dce3
💬 TheCharlatan commented on pull request "init: Fixes for file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/27539#issuecomment-2089013169)
This was rebased fairly recently, but you have not addressed this open comment https://github.com/bitcoin/bitcoin/pull/27539#discussion_r1203666492 . Are you still working on this?
(https://github.com/bitcoin/bitcoin/pull/27539#issuecomment-2089013169)
This was rebased fairly recently, but you have not addressed this open comment https://github.com/bitcoin/bitcoin/pull/27539#discussion_r1203666492 . Are you still working on this?
💬 achow101 commented on pull request "system: use %LOCALAPPDATA% as default datadir on windows":
(https://github.com/bitcoin/bitcoin/pull/27064#issuecomment-2089055814)
ACK 84900ac34f6888b7a851d0a6a5885192155f865c
Tested on Windows. `%APPDATA%/Bitcoin` continues to be used if it exists, `%LOCALAPPDATA%` if not.
(https://github.com/bitcoin/bitcoin/pull/27064#issuecomment-2089055814)
ACK 84900ac34f6888b7a851d0a6a5885192155f865c
Tested on Windows. `%APPDATA%/Bitcoin` continues to be used if it exists, `%LOCALAPPDATA%` if not.
💬 achow101 commented on pull request "rpc: return warnings as an array instead of just a single one":
(https://github.com/bitcoin/bitcoin/pull/29845#issuecomment-2089058662)
re-ACK 42fb5311b19582361409d65c6fddeadbee14bb97
(https://github.com/bitcoin/bitcoin/pull/29845#issuecomment-2089058662)
re-ACK 42fb5311b19582361409d65c6fddeadbee14bb97
🤔 sr-gi reviewed a pull request: "net: don't lock cs_main while reading blocks in net processing"
(https://github.com/bitcoin/bitcoin/pull/26326#pullrequestreview-2034387611)
Concept ACK
I agree with @furszy in https://github.com/bitcoin/bitcoin/pull/26326#discussion_r1381776328 that we should followup this with sending NotFounds for all the blocks that we are unable (or unwilling) to provide
(https://github.com/bitcoin/bitcoin/pull/26326#pullrequestreview-2034387611)
Concept ACK
I agree with @furszy in https://github.com/bitcoin/bitcoin/pull/26326#discussion_r1381776328 that we should followup this with sending NotFounds for all the blocks that we are unable (or unwilling) to provide
💬 sipa commented on pull request "net: don't lock cs_main while reading blocks in net processing":
(https://github.com/bitcoin/bitcoin/pull/26326#issuecomment-2089075429)
> we should followup this with sending NotFounds for all the blocks that we are unable (or unwilling) to provide
We can't just change the protocol like that; other software than Bitcoin Core might not expect `notfound` for block messages. But I do believe it would be useful to have *some* way of signalling, and using the information, that a block is unavailable. It would need a short BIP, and a way to negotiate support, but it would mean that over time we may also be able to prefer downloadin
...
(https://github.com/bitcoin/bitcoin/pull/26326#issuecomment-2089075429)
> we should followup this with sending NotFounds for all the blocks that we are unable (or unwilling) to provide
We can't just change the protocol like that; other software than Bitcoin Core might not expect `notfound` for block messages. But I do believe it would be useful to have *some* way of signalling, and using the information, that a block is unavailable. It would need a short BIP, and a way to negotiate support, but it would mean that over time we may also be able to prefer downloadin
...
💬 IAmAdamRest commented on issue "Manually Banning Peers Results in Crash":
(https://github.com/bitcoin/bitcoin/issues/29916#issuecomment-2089079287)
I decided to try on my ubuntu machine as well and had this same issue but I am not a power user by any means. I will look up this gdb thing to see if I can get a log.

happened right after i banned a peer that kept connecting and disconnecting and connecting and disconnecting like hundreds of times over the course of a few minutes. each time the cpu would get high and i had enough it was an
...
(https://github.com/bitcoin/bitcoin/issues/29916#issuecomment-2089079287)
I decided to try on my ubuntu machine as well and had this same issue but I am not a power user by any means. I will look up this gdb thing to see if I can get a log.

happened right after i banned a peer that kept connecting and disconnecting and connecting and disconnecting like hundreds of times over the course of a few minutes. each time the cpu would get high and i had enough it was an
...
💬 sr-gi commented on pull request "fuzz: txorphan tests fixups":
(https://github.com/bitcoin/bitcoin/pull/29974#discussion_r1586788475)
The reason why I choose to do it this way is because I don't think we generally want to be generating the same transaction often, but it won't be an issue if the case is hit once in a while (really infrequently really).
I don't have strong opinions on what the odds for that should be, so I'm happy to change it if someone does, otherwise I'll leave it as is
(https://github.com/bitcoin/bitcoin/pull/29974#discussion_r1586788475)
The reason why I choose to do it this way is because I don't think we generally want to be generating the same transaction often, but it won't be an issue if the case is hit once in a while (really infrequently really).
I don't have strong opinions on what the odds for that should be, so I'm happy to change it if someone does, otherwise I'll leave it as is
⚠️ laanwj opened an issue: "Linux debug information for release is broken"
(https://github.com/bitcoin/bitcoin/issues/30016)
The debug information in the shipped `.dbg` artifacts seems to be unusable. The `.debug_info` section is corrupt:
```sh
$ tar -zxvf bitcoin-27.0-x86_64-linux-gnu-debug.tar.gz
$ objdump -Wi bitcoin-27.0/bin/bitcoin-cli.dbg
Section '.debug_info' has an invalid size: 0.
bitcoin-27.0/bin/bitcoin-cli.dbg: file format elf64-x86-64
Section '.debug_info' has an invalid size: 0.
```
i've checked various Linux architectures (at least x86_64, ARM, RISC-V) and the same issue exists there.
...
(https://github.com/bitcoin/bitcoin/issues/30016)
The debug information in the shipped `.dbg` artifacts seems to be unusable. The `.debug_info` section is corrupt:
```sh
$ tar -zxvf bitcoin-27.0-x86_64-linux-gnu-debug.tar.gz
$ objdump -Wi bitcoin-27.0/bin/bitcoin-cli.dbg
Section '.debug_info' has an invalid size: 0.
bitcoin-27.0/bin/bitcoin-cli.dbg: file format elf64-x86-64
Section '.debug_info' has an invalid size: 0.
```
i've checked various Linux architectures (at least x86_64, ARM, RISC-V) and the same issue exists there.
...
💬 laanwj commented on issue "Linux debug information for release is broken":
(https://github.com/bitcoin/bitcoin/issues/30016#issuecomment-2089161664)
Okay, may be a `objdump` bug actually?
`readelf -wi` (which should be equivalent to `objdump -Wi`) parses the same section fine. So does `gdb` it seems?
(FWIW i did try various versions of `objdump` before reporting this)
(https://github.com/bitcoin/bitcoin/issues/30016#issuecomment-2089161664)
Okay, may be a `objdump` bug actually?
`readelf -wi` (which should be equivalent to `objdump -Wi`) parses the same section fine. So does `gdb` it seems?
(FWIW i did try various versions of `objdump` before reporting this)
✅ laanwj closed an issue: "Linux debug information for release is broken"
(https://github.com/bitcoin/bitcoin/issues/30016)
(https://github.com/bitcoin/bitcoin/issues/30016)
💬 laanwj commented on issue "Linux debug information for release is broken":
(https://github.com/bitcoin/bitcoin/issues/30016#issuecomment-2089162681)
Closing for now.
(https://github.com/bitcoin/bitcoin/issues/30016#issuecomment-2089162681)
Closing for now.
💬 cbergqvist commented on pull request "net: Favor peers from addrman over fetching seednodes":
(https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1586817396)
Accidental indentation change?
(https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1586817396)
Accidental indentation change?
🤔 cbergqvist requested changes to a pull request: "net: Favor peers from addrman over fetching seednodes"
(https://github.com/bitcoin/bitcoin/pull/29605#pullrequestreview-2034448189)
Re-tested 33ddd1b4c1cb1165b5068fbf7a9461e295f6cef1. Bumping of timeout seems blocking.
(https://github.com/bitcoin/bitcoin/pull/29605#pullrequestreview-2034448189)
Re-tested 33ddd1b4c1cb1165b5068fbf7a9461e295f6cef1. Bumping of timeout seems blocking.
💬 cbergqvist commented on pull request "net: Favor peers from addrman over fetching seednodes":
(https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1586847044)
Switched to using `Span` for a slightly smaller change suggestion in f9bfc588f2e81a7febe233f591b75e41a52db8b4.
(https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1586847044)
Switched to using `Span` for a slightly smaller change suggestion in f9bfc588f2e81a7febe233f591b75e41a52db8b4.
💬 cbergqvist commented on pull request "net: Favor peers from addrman over fetching seednodes":
(https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1586850195)
Had to bump the timeout 20 -> 30 seconds for the test to pass. Example commit with extra logging: 46bac29032c0148ce5288faadbcdc3961d522060
(https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1586850195)
Had to bump the timeout 20 -> 30 seconds for the test to pass. Example commit with extra logging: 46bac29032c0148ce5288faadbcdc3961d522060
💬 Symphonic3 commented on issue "Change estimate_mode default to "ECONOMICAL" in these RPC calls":
(https://github.com/bitcoin/bitcoin/issues/30009#issuecomment-2089182279)
Willing to create a PR for this if desired!
(https://github.com/bitcoin/bitcoin/issues/30009#issuecomment-2089182279)
Willing to create a PR for this if desired!
👍 cbergqvist approved a pull request: "test: Handle functional test disk-full error"
(https://github.com/bitcoin/bitcoin/pull/29335#pullrequestreview-2034535106)
reACK 357ad110548d726021547d85b5b2bfcf3191d7e3. Looks good!
Tested on RAM disk of 1GB, 1430MB (`doas mount -t tmpfs -o size=1430m tmpfs /mnt/tmp/`) and varied the `--jobs` and `--nocleanup`, observing expected warnings + error on disk full.
(https://github.com/bitcoin/bitcoin/pull/29335#pullrequestreview-2034535106)
reACK 357ad110548d726021547d85b5b2bfcf3191d7e3. Looks good!
Tested on RAM disk of 1GB, 1430MB (`doas mount -t tmpfs -o size=1430m tmpfs /mnt/tmp/`) and varied the `--jobs` and `--nocleanup`, observing expected warnings + error on disk full.
💬 AngusP commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1586884665)
(Ignore me if I'm understanding, still picking things up 😅) -- coinbase transactions are [required to have all `0` Wtxids](https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#commitment-structure) (`0x0000....0000`) which means for an `INV` message for a witness coinbase you have to use the non-witness txid to reference the transaction? - so `AlreadyHaveTx` in that case is explicitly *not* using a Wtxid? (Or are coinbases never `INV`'d?)
Also perhaps not great/confusing to treat W
...
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1586884665)
(Ignore me if I'm understanding, still picking things up 😅) -- coinbase transactions are [required to have all `0` Wtxids](https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#commitment-structure) (`0x0000....0000`) which means for an `INV` message for a witness coinbase you have to use the non-witness txid to reference the transaction? - so `AlreadyHaveTx` in that case is explicitly *not* using a Wtxid? (Or are coinbases never `INV`'d?)
Also perhaps not great/confusing to treat W
...