💬 alfonsoromanz commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1727195864)
Done
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1727195864)
Done
💬 achow101 commented on pull request "[WIP] seeds: Add additional seed source and bump uptime requirements for Onion and I2P nodes":
(https://github.com/bitcoin/bitcoin/pull/30695#discussion_r1727198083)
This will overwrite instead of append.
(https://github.com/bitcoin/bitcoin/pull/30695#discussion_r1727198083)
This will overwrite instead of append.
💬 alfonsoromanz commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1727198919)
Done
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1727198919)
Done
💬 alfonsoromanz commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1727199409)
Done. Thanks!
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1727199409)
Done. Thanks!
💬 Sjors commented on issue "release: ship codesigned MacOS arm64 binaries":
(https://github.com/bitcoin/bitcoin/issues/29749#issuecomment-2304877723)
Getting some feedback from the Stratum v2 folks that it would be useful to have codesigned macOS `bitcoind` and `bitcoin-cli`, both for integration tests and to make workshops easier.
(https://github.com/bitcoin/bitcoin/issues/29749#issuecomment-2304877723)
Getting some feedback from the Stratum v2 folks that it would be useful to have codesigned macOS `bitcoind` and `bitcoin-cli`, both for integration tests and to make workshops easier.
📝 ismaelsadeeq opened a pull request: "Wallet, Bugfix: Lock Wallet Context `cs` Before Adding/Removing Settings"
(https://github.com/bitcoin/bitcoin/pull/30697)
This PR fixes #30620.
As described in the issue:
"Creating two wallets with `load_on_startup=true` simultaneously results in only one of the wallets being added to the startup file."
This issue is most likely because the wallet does not lock any mutex before modifying the wallet settings, leading to a race condition when multiple threads call `AddWalletSetting` or `RemoveWalletSetting`.
This can be fixed by locking the wallet context `cs` in both `AddWalletSetting` and `RemoveWalletSetti
...
(https://github.com/bitcoin/bitcoin/pull/30697)
This PR fixes #30620.
As described in the issue:
"Creating two wallets with `load_on_startup=true` simultaneously results in only one of the wallets being added to the startup file."
This issue is most likely because the wallet does not lock any mutex before modifying the wallet settings, leading to a race condition when multiple threads call `AddWalletSetting` or `RemoveWalletSetting`.
This can be fixed by locking the wallet context `cs` in both `AddWalletSetting` and `RemoveWalletSetti
...
💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1727240719)
No. I just had to google this magic number and felt slightly rickrolled
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1727240719)
No. I just had to google this magic number and felt slightly rickrolled
💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1727243654)
this fails if we have a package to validate...
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1727243654)
this fails if we have a package to validate...
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1727249636)
ohhh i get it now. i thought you found a crash and were giving a cryptic description of the input
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1727249636)
ohhh i get it now. i thought you found a crash and were giving a cryptic description of the input
💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2304916648)
`3yTMzMzMzMzMzMzMzMw2lcV+LVVYVVX//y1maZY/xcWVxX5VWFVVVVUlugr///8tZmlVCg==`
if you add `TX_RECONSIDERABLE,` to the end of `TESTED_TX_RESULTS`, this should crash because it found a package to try
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2304916648)
`3yTMzMzMzMzMzMzMzMw2lcV+LVVYVVX//y1maZY/xcWVxX5VWFVVVVUlugr///8tZmlVCg==`
if you add `TX_RECONSIDERABLE,` to the end of `TESTED_TX_RESULTS`, this should crash because it found a package to try
💬 maflcko commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#issuecomment-2304924301)
(Assigned 28.0, but this is not a regression, so not a blocker for rc1. I think it can be backported to rc2, just in case it needs more time.)
(https://github.com/bitcoin/bitcoin/pull/30697#issuecomment-2304924301)
(Assigned 28.0, but this is not a regression, so not a blocker for rc1. I think it can be backported to rc2, just in case it needs more time.)
👍 ryanofsky approved a pull request: "refactor: Replace ParseHex with consteval ""_hex literals"
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2254756097)
Code review ACK 7a4d249267cb5f63ace96a5fcc03452acc5468b5. Changes since last review were just adding hex_literals namespace and improving comments as suggested (thanks!)
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2254756097)
Code review ACK 7a4d249267cb5f63ace96a5fcc03452acc5468b5. Changes since last review were just adding hex_literals namespace and improving comments as suggested (thanks!)
💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1727209841)
In commit "util: Add consteval ""_hex[_v][_u8] literals" (b8b10cea4f4331b5f587cfee6ac401fbbf0697a5)
I think this comment is still missing a lot of important information. It doesn't mention that this is an alternative to ParseHex, and only mentions one of variant suffixes below, not describing the real considerations required to choose between these alternatives. The commit message does have this information, but it is unlikely someone trying use these functions will see the commit message. Wo
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1727209841)
In commit "util: Add consteval ""_hex[_v][_u8] literals" (b8b10cea4f4331b5f587cfee6ac401fbbf0697a5)
I think this comment is still missing a lot of important information. It doesn't mention that this is an alternative to ParseHex, and only mentions one of variant suffixes below, not describing the real considerations required to choose between these alternatives. The commit message does have this information, but it is unlikely someone trying use these functions will see the commit message. Wo
...
💬 maflcko commented on issue "Unit test failures when using multiple jobs and RANDOM_CTX_SEED":
(https://github.com/bitcoin/bitcoin/issues/30696#issuecomment-2304975003)
Nice fine. `RANDOM_CTX_SEED` is only really for debugging single unit tests after the fact that they failed, but it would be nice to fix this nonetheless.
(https://github.com/bitcoin/bitcoin/issues/30696#issuecomment-2304975003)
Nice fine. `RANDOM_CTX_SEED` is only really for debugging single unit tests after the fact that they failed, but it would be nice to fix this nonetheless.
💬 maflcko commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1727334279)
Makes sense! I'll do the documentation cleanup in a follow-up, if that is fine, because this refactoring pull request doesn't change the behavior around this function, and I am trying to keep the scope limited.
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1727334279)
Makes sense! I'll do the documentation cleanup in a follow-up, if that is fine, because this refactoring pull request doesn't change the behavior around this function, and I am trying to keep the scope limited.
💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1727346780)
`g_best_block_cv.notify_all();` is called by `Chainstate::UpdateTip()`, which indeed is called by `ConnectTip` and `DisconnectTip()`. Still wrapping my head around what the previous approach was. I doubt these RPC's were used outside of tests, but it would make sense to document the different.
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1727346780)
`g_best_block_cv.notify_all();` is called by `Chainstate::UpdateTip()`, which indeed is called by `ConnectTip` and `DisconnectTip()`. Still wrapping my head around what the previous approach was. I doubt these RPC's were used outside of tests, but it would make sense to document the different.
💬 hebasto commented on pull request "build: Mark `x86_64-linux-gnu` release binaries as CET-enabled":
(https://github.com/bitcoin/bitcoin/pull/30685#issuecomment-2305056964)
> Guix building [89f8e8e](https://github.com/bitcoin/bitcoin/commit/89f8e8e7f5828b1d06925e036c16eda050b05c81) on `aarch64` doesn't work:
The issue should fixed now.
(https://github.com/bitcoin/bitcoin/pull/30685#issuecomment-2305056964)
> Guix building [89f8e8e](https://github.com/bitcoin/bitcoin/commit/89f8e8e7f5828b1d06925e036c16eda050b05c81) on `aarch64` doesn't work:
The issue should fixed now.
📝 ismaelsadeeq converted_to_draft a pull request: "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings"
(https://github.com/bitcoin/bitcoin/pull/30697)
This PR fixes #30620.
As described in the issue:
"Creating two wallets with `load_on_startup=true` simultaneously results in only one of the wallets being added to the startup file."
This issue is most likely because the wallet does not lock any mutex before modifying the wallet settings, leading to a race condition when multiple threads call `AddWalletSetting` or `RemoveWalletSetting`.
This can be fixed by locking the wallet context `cs` in both `AddWalletSetting` and `RemoveWalletSetti
...
(https://github.com/bitcoin/bitcoin/pull/30697)
This PR fixes #30620.
As described in the issue:
"Creating two wallets with `load_on_startup=true` simultaneously results in only one of the wallets being added to the startup file."
This issue is most likely because the wallet does not lock any mutex before modifying the wallet settings, leading to a race condition when multiple threads call `AddWalletSetting` or `RemoveWalletSetting`.
This can be fixed by locking the wallet context `cs` in both `AddWalletSetting` and `RemoveWalletSetti
...
💬 ismaelsadeeq commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#issuecomment-2305069745)
In draft now before addressing TSAN failure https://github.com/bitcoin/bitcoin/pull/30697/checks?check_run_id=29119417676
(https://github.com/bitcoin/bitcoin/pull/30697#issuecomment-2305069745)
In draft now before addressing TSAN failure https://github.com/bitcoin/bitcoin/pull/30697/checks?check_run_id=29119417676
🤔 marcofleon reviewed a pull request: "kernel: pre-28.x chainparams and headerssync update"
(https://github.com/bitcoin/bitcoin/pull/30658#pullrequestreview-2255005506)
ACK 221809b81cfcecb04050915eebacffda2599da42
Headerssync params lgtm, got the same ones. I checked the main chain params against my node and the only thing is that my blocks dir is 675 gbs, not 620. Not sure if it matters too much.
(https://github.com/bitcoin/bitcoin/pull/30658#pullrequestreview-2255005506)
ACK 221809b81cfcecb04050915eebacffda2599da42
Headerssync params lgtm, got the same ones. I checked the main chain params against my node and the only thing is that my blocks dir is 675 gbs, not 620. Not sure if it matters too much.