💬 ryanofsky commented on pull request "wallet: Fix migration of wallets with txs that have both spendable and watchonly outputs":
(https://github.com/bitcoin/bitcoin/pull/28868#discussion_r1473532556)
In commit "wallet: Reload the wallet if migration exited early" (d8002ed333e13e54726bcd922303cb1e42544b44)
I don't think it is good to move `reload_wallet` here before `BackupWallet` is called. It seems like it would make it easy to add an innocent call to `reload_wallet(local_wallet)` that looks like it is just reloading the wallet, but doesn't return right away, so the wallet get added to `wallet_dirs` before it is backed up and possibly deleted as part of the `fs::remove_all` loop.
Most
...
(https://github.com/bitcoin/bitcoin/pull/28868#discussion_r1473532556)
In commit "wallet: Reload the wallet if migration exited early" (d8002ed333e13e54726bcd922303cb1e42544b44)
I don't think it is good to move `reload_wallet` here before `BackupWallet` is called. It seems like it would make it easy to add an innocent call to `reload_wallet(local_wallet)` that looks like it is just reloading the wallet, but doesn't return right away, so the wallet get added to `wallet_dirs` before it is backed up and possibly deleted as part of the `fs::remove_all` loop.
Most
...
💬 amitiuttarwar commented on pull request "addrman: delete addresses that don't belong to the supported networks":
(https://github.com/bitcoin/bitcoin/pull/29330#issuecomment-1920125508)
+1 to the questions asked by @naumenkogs. I agree with the statement in the OP that this change would "avoid a lot of unnecessary addrman `Select` calls (especially when...", but would like additional context as to how that would tangibly impact the node operations or end user.
in general I am hesitant to add init params since it increases the combinatoric space of startup possibilities & they are hard to deprecate over time. maybe one idea (if we want to support this feature) could be to ad
...
(https://github.com/bitcoin/bitcoin/pull/29330#issuecomment-1920125508)
+1 to the questions asked by @naumenkogs. I agree with the statement in the OP that this change would "avoid a lot of unnecessary addrman `Select` calls (especially when...", but would like additional context as to how that would tangibly impact the node operations or end user.
in general I am hesitant to add init params since it increases the combinatoric space of startup possibilities & they are hard to deprecate over time. maybe one idea (if we want to support this feature) could be to ad
...
💬 1440000bytes commented on pull request "addrman: delete addresses that don't belong to the supported networks":
(https://github.com/bitcoin/bitcoin/pull/29330#issuecomment-1920304941)
> This PR adds a new flag -cleanupaddrman. When initializing a node with this flag, it will delete addresses from addrman (both new and tried tables) that do not belong to the supported networks (e.g. -onlynet).
I don't think this is a good idea
> This flag is not enabled by default.
Concept ACK because it's not enabled by default and could be useful for testing.
(https://github.com/bitcoin/bitcoin/pull/29330#issuecomment-1920304941)
> This PR adds a new flag -cleanupaddrman. When initializing a node with this flag, it will delete addresses from addrman (both new and tried tables) that do not belong to the supported networks (e.g. -onlynet).
I don't think this is a good idea
> This flag is not enabled by default.
Concept ACK because it's not enabled by default and could be useful for testing.
💬 tdb3 commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1473699881)
Pulled changes (771d1e1d206efe687b8661ab966cc1a62cc7ba39), built, ran all functional tests (all passed). Re-ran the following on 771d1e1d206efe687b8661ab966cc1a62cc7ba39 and on release v26.0.
## Action (1): __Unexpected difference__
`curl --user test --data-binary '[]' -H 'content-type: text/plain;' http://127.0.0.1:38332/`
Result: Unexpected difference
v26.0 behavior: `[]`
PR 27101 (771d1e1d206efe687b8661ab966cc1a62cc7ba39) behavior: (no content) (in alignment with 2.0 spec, b
...
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1473699881)
Pulled changes (771d1e1d206efe687b8661ab966cc1a62cc7ba39), built, ran all functional tests (all passed). Re-ran the following on 771d1e1d206efe687b8661ab966cc1a62cc7ba39 and on release v26.0.
## Action (1): __Unexpected difference__
`curl --user test --data-binary '[]' -H 'content-type: text/plain;' http://127.0.0.1:38332/`
Result: Unexpected difference
v26.0 behavior: `[]`
PR 27101 (771d1e1d206efe687b8661ab966cc1a62cc7ba39) behavior: (no content) (in alignment with 2.0 spec, b
...
💬 tdb3 commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1473700874)
## Action (5): __Successfully executed response, legacy behavior retained__
`curl --user test --data-binary '{"jsonrpc":"1.0","method":"getblockchaininfo","id":"tester"}' -H 'content-type: text/plain;' http://127.0.0.1:38332/`
Result: Successfully executed response
v26.0 behavior: `{"result":{"chain":"signet","blocks":4320,"headers":4320,"bestblockhash":"0000013e07d388dd5c8cb66c990caa546fc04e7533cad18c27ed3d603f37a6c0","difficulty":0.001127075738664131,"time":1706750596,"mediantime":17067
...
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1473700874)
## Action (5): __Successfully executed response, legacy behavior retained__
`curl --user test --data-binary '{"jsonrpc":"1.0","method":"getblockchaininfo","id":"tester"}' -H 'content-type: text/plain;' http://127.0.0.1:38332/`
Result: Successfully executed response
v26.0 behavior: `{"result":{"chain":"signet","blocks":4320,"headers":4320,"bestblockhash":"0000013e07d388dd5c8cb66c990caa546fc04e7533cad18c27ed3d603f37a6c0","difficulty":0.001127075738664131,"time":1706750596,"mediantime":17067
...
⚠️ starius opened an issue: "build warnings in outputtype.cpp: may be used uninitialized"
(https://github.com/bitcoin/bitcoin/issues/29359)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
Building Bitcoin Core from source using official instructions from https://github.com/bitcoin/bitcoin/blob/master/doc/build-unix.md I got compilation warnings, complaining about may be used uninitialized variables in two destructors.
Log of `make -j 10`:
[bitcoin-compilation-warning-master.txt](https://github.com/bitcoin/bitcoin/files/14119565/bitcoin-compilation-warning-master.txt)
...
(https://github.com/bitcoin/bitcoin/issues/29359)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
Building Bitcoin Core from source using official instructions from https://github.com/bitcoin/bitcoin/blob/master/doc/build-unix.md I got compilation warnings, complaining about may be used uninitialized variables in two destructors.
Log of `make -j 10`:
[bitcoin-compilation-warning-master.txt](https://github.com/bitcoin/bitcoin/files/14119565/bitcoin-compilation-warning-master.txt)
...
💬 hernanmarino commented on pull request "Modify command line help to show support for BIP21 URIs":
(https://github.com/bitcoin-core/gui/pull/752#discussion_r1473761422)
Done
(https://github.com/bitcoin-core/gui/pull/752#discussion_r1473761422)
Done
💬 hernanmarino commented on pull request "Modify command line help to show support for BIP21 URIs":
(https://github.com/bitcoin-core/gui/pull/752#discussion_r1473761491)
Done
(https://github.com/bitcoin-core/gui/pull/752#discussion_r1473761491)
Done
💬 hernanmarino commented on pull request "Modify command line help to show support for BIP21 URIs":
(https://github.com/bitcoin-core/gui/pull/752#issuecomment-1920456532)
Fixed the 2 typos mentioned by @luke-jr and @kristapsk . Also rebased for CI.
(https://github.com/bitcoin-core/gui/pull/752#issuecomment-1920456532)
Fixed the 2 typos mentioned by @luke-jr and @kristapsk . Also rebased for CI.
💬 hernanmarino commented on pull request "Modify command line help to show support for BIP21 URIs":
(https://github.com/bitcoin-core/gui/pull/752#issuecomment-1920461069)
Fixed the 2 typos mentioned by @luke-jr and @kristapsk . Also rebased for CI.
(https://github.com/bitcoin-core/gui/pull/752#issuecomment-1920461069)
Fixed the 2 typos mentioned by @luke-jr and @kristapsk . Also rebased for CI.
💬 stratospher commented on pull request "test: use v2 everywhere for P2PConnection if --v2transport is enabled":
(https://github.com/bitcoin/bitcoin/pull/29358#issuecomment-1920563086)
> Hmm, "test each commit fails" The problem is that commit https://github.com/bitcoin/bitcoin/commit/606f4f32014f029a6999d7f94b7231fefafdf55f (which switches P2PConnection to v2 by default) is enabled last. It might be best to squash everything into one big commit (even if that might make review a bit harder) or I'd have to add some temporary workarounds. Opinions?
would reordering the commits to keep the last commit as first commit work? reviewing is definitely easier with separate commits.
(https://github.com/bitcoin/bitcoin/pull/29358#issuecomment-1920563086)
> Hmm, "test each commit fails" The problem is that commit https://github.com/bitcoin/bitcoin/commit/606f4f32014f029a6999d7f94b7231fefafdf55f (which switches P2PConnection to v2 by default) is enabled last. It might be best to squash everything into one big commit (even if that might make review a bit harder) or I'd have to add some temporary workarounds. Opinions?
would reordering the commits to keep the last commit as first commit work? reviewing is definitely easier with separate commits.
💬 maflcko commented on issue "build warnings in outputtype.cpp: may be used uninitialized":
(https://github.com/bitcoin/bitcoin/issues/29359#issuecomment-1920779707)
I guess regardless of the g++ version used on Ubuntu or Debian, some kind of false positive warnings will be printed. I wonder why g++ on other distros does not print those warnings.
(https://github.com/bitcoin/bitcoin/issues/29359#issuecomment-1920779707)
I guess regardless of the g++ version used on Ubuntu or Debian, some kind of false positive warnings will be printed. I wonder why g++ on other distros does not print those warnings.
👍 MarnixCroes approved a pull request: "Modify command line help to show support for BIP21 URIs"
(https://github.com/bitcoin-core/gui/pull/752#pullrequestreview-1855939371)
ack ede5014c445dcb40ddcfdede2c51236bbfe85f5e
(https://github.com/bitcoin-core/gui/pull/752#pullrequestreview-1855939371)
ack ede5014c445dcb40ddcfdede2c51236bbfe85f5e
💬 maflcko commented on pull request "log: Nuke error(...)":
(https://github.com/bitcoin/bitcoin/pull/29236#issuecomment-1920925835)
rebased
(https://github.com/bitcoin/bitcoin/pull/29236#issuecomment-1920925835)
rebased
⚠️ maflcko opened an issue: "ci: Android NDK has too old libc++"
(https://github.com/bitcoin/bitcoin/issues/29360)
The android CI task fails because the embedded libc++ in the NDK is too old.
Can it be bumped?
(https://github.com/bitcoin/bitcoin/issues/29360)
The android CI task fails because the embedded libc++ in the NDK is too old.
Can it be bumped?
💬 maflcko commented on issue "ci: Android NDK has too old libc++":
(https://github.com/bitcoin/bitcoin/issues/29360#issuecomment-1920972420)
Reference:
```
./util/fs.h:65:30: error: no matching constructor for initialization of 'const std::u8string &' (aka 'const basic_string<char8_t> &')
const std::u8string& utf8_str{std::filesystem::path::u8string()};
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/ci_container_base/depends/SDKs/android/ndk/23.2.8568313/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/c++/v1/string:794:40: note: candidate constructor not viable: no known conv
...
(https://github.com/bitcoin/bitcoin/issues/29360#issuecomment-1920972420)
Reference:
```
./util/fs.h:65:30: error: no matching constructor for initialization of 'const std::u8string &' (aka 'const basic_string<char8_t> &')
const std::u8string& utf8_str{std::filesystem::path::u8string()};
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/ci_container_base/depends/SDKs/android/ndk/23.2.8568313/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/c++/v1/string:794:40: note: candidate constructor not viable: no known conv
...
👍 kristapsk approved a pull request: "Modify command line help to show support for BIP21 URIs"
(https://github.com/bitcoin-core/gui/pull/752#pullrequestreview-1856201976)
utACK ede5014c445dcb40ddcfdede2c51236bbfe85f5e
(https://github.com/bitcoin-core/gui/pull/752#pullrequestreview-1856201976)
utACK ede5014c445dcb40ddcfdede2c51236bbfe85f5e
⚠️ Xaspr reopened an issue: "Unable to sync blockchain on laptop: ERROR: ReadBlockFromDisk: Deserialize or I/O error"
(https://github.com/bitcoin/bitcoin/issues/29255)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
After installing Bitcoin Core 26.0 in Windows Pro 11 and starting IBD, Core crashes.
I know Windows might not be exactly ideal, but I like to run a pruned node on my work laptop as well. I didn't set pruning yet by the way, I started Core just with the standard settings.
Bitcoin Core crashes and the following error pops up in debug.log:
`ERROR: ReadBlockFromDisk: Deserialize or
...
(https://github.com/bitcoin/bitcoin/issues/29255)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
After installing Bitcoin Core 26.0 in Windows Pro 11 and starting IBD, Core crashes.
I know Windows might not be exactly ideal, but I like to run a pruned node on my work laptop as well. I didn't set pruning yet by the way, I started Core just with the standard settings.
Bitcoin Core crashes and the following error pops up in debug.log:
`ERROR: ReadBlockFromDisk: Deserialize or
...
💬 Xaspr commented on issue "Unable to sync blockchain on laptop: ERROR: ReadBlockFromDisk: Deserialize or I/O error":
(https://github.com/bitcoin/bitcoin/issues/29255#issuecomment-1921081600)
Not sure if related, but I'm still running into issues. I've tried a lot by now, from changing antivirus program, disabling antivirus and anti-malware, checking CPU temperature, checking SSD integrity, updating firmware, updating BIOS, trying older versions of Bitcoin Core and more.
This specific data corruption error hasn't come up the last few attempts to get to IBD. But often after stopping Core and starting it up again, I get the message that I should reindex. This seems to be a problem
...
(https://github.com/bitcoin/bitcoin/issues/29255#issuecomment-1921081600)
Not sure if related, but I'm still running into issues. I've tried a lot by now, from changing antivirus program, disabling antivirus and anti-malware, checking CPU temperature, checking SSD integrity, updating firmware, updating BIOS, trying older versions of Bitcoin Core and more.
This specific data corruption error hasn't come up the last few attempts to get to IBD. But often after stopping Core and starting it up again, I get the message that I should reindex. This seems to be a problem
...
💬 maflcko commented on issue "Unable to sync blockchain on laptop: ERROR: ReadBlockFromDisk: Deserialize or I/O error":
(https://github.com/bitcoin/bitcoin/issues/29255#issuecomment-1921094517)
> It's may still be antivirus software that's persistent in interfering with Core.
It would be good to check this. If you have a completely separate machine, you could try to install the "anti"-virus software there and see if the issue happens?
Other than that, I believe this is a caused by overheating. It may be possible that the CPU itself happens to work under high heat as part of a benchmark or stress test. Also, other hardware components are fine by themselves as part of a benchmark o
...
(https://github.com/bitcoin/bitcoin/issues/29255#issuecomment-1921094517)
> It's may still be antivirus software that's persistent in interfering with Core.
It would be good to check this. If you have a completely separate machine, you could try to install the "anti"-virus software there and see if the issue happens?
Other than that, I believe this is a caused by overheating. It may be possible that the CPU itself happens to work under high heat as part of a benchmark or stress test. Also, other hardware components are fine by themselves as part of a benchmark o
...
📝 maflcko opened a pull request: "refactor: Fix timedata includes"
(https://github.com/bitcoin/bitcoin/pull/29361)
Remove unused includes. Also, fixup comments, see https://github.com/bitcoin/bitcoin/pull/28956/files#r1464827885. Also, add missing includes to `chain.h` while touching it.
(https://github.com/bitcoin/bitcoin/pull/29361)
Remove unused includes. Also, fixup comments, see https://github.com/bitcoin/bitcoin/pull/28956/files#r1464827885. Also, add missing includes to `chain.h` while touching it.