💬 sipa commented on pull request "Limit retries in GetRNDRRS to avoid infinite loop":
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1947836644)
When this fails, do we want to fall back to `GetRNSR` instead, rather than returning whatever arbitrary value was left in the register?
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1947836644)
When this fails, do we want to fall back to `GetRNSR` instead, rather than returning whatever arbitrary value was left in the register?
💬 Christewart commented on pull request "test: Add `leaf_version` parameter to `taproot_tree_helper()`":
(https://github.com/bitcoin/bitcoin/pull/29371#issuecomment-2645752238)
Rebased
(https://github.com/bitcoin/bitcoin/pull/29371#issuecomment-2645752238)
Rebased
💬 Christewart commented on pull request "consensus: Consistently encode and decode `OP_1NEGATE` similar to other small ints in Script":
(https://github.com/bitcoin/bitcoin/pull/29589#issuecomment-2645753696)
Rebased
(https://github.com/bitcoin/bitcoin/pull/29589#issuecomment-2645753696)
Rebased
💬 maflcko commented on pull request "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows":
(https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2645774491)
> Give `cs_main` hidden visiblility and add a `get_csmain()` getter function for bitcon-chainstate to use. That would work but would mean that bitcoin-chainstate's code would diverge from the rest of the codebase. (assuming we didn't do a global replacement, which would be a HUGE hammer)
(Probably out-of-scope for this pull, but there is already a getter for cs_main: `chainman.GetMutex()`, which can be used today, and can make it easier to turn cs_main from a global variable into a member fie
...
(https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2645774491)
> Give `cs_main` hidden visiblility and add a `get_csmain()` getter function for bitcon-chainstate to use. That would work but would mean that bitcoin-chainstate's code would diverge from the rest of the codebase. (assuming we didn't do a global replacement, which would be a HUGE hammer)
(Probably out-of-scope for this pull, but there is already a getter for cs_main: `chainman.GetMutex()`, which can be used today, and can make it easier to turn cs_main from a global variable into a member fie
...
✅ maflcko closed a pull request: "fixing-link-Update dependencies.md"
(https://github.com/bitcoin/bitcoin/pull/31822)
(https://github.com/bitcoin/bitcoin/pull/31822)
💬 maflcko commented on pull request "fixing-link-Update dependencies.md":
(https://github.com/bitcoin/bitcoin/pull/31822#issuecomment-2645783216)
I wouldn't mind the change and would be happy to review it, but given the two conflicts https://github.com/bitcoin/bitcoin/pull/31822#issuecomment-2643522687 (one which also rewords the file, and another one which removes BDB and the line changed in this pull request anyway), I'll be closing this for now. Thanks.
(https://github.com/bitcoin/bitcoin/pull/31822#issuecomment-2645783216)
I wouldn't mind the change and would be happy to review it, but given the two conflicts https://github.com/bitcoin/bitcoin/pull/31822#issuecomment-2643522687 (one which also rewords the file, and another one which removes BDB and the line changed in this pull request anyway), I'll be closing this for now. Thanks.
💬 tofutim commented on issue "Unable to cross compile on linux for macos (28.x branch)":
(https://github.com/bitcoin/bitcoin/issues/31027#issuecomment-2645915233)
> Update: re-downloaded Xcode and re-extracted the SDK, guix build is proceeding again
Did it work after re-extracting the SDK? I had no luck with 28.1. Trying 27.2 now.
(https://github.com/bitcoin/bitcoin/issues/31027#issuecomment-2645915233)
> Update: re-downloaded Xcode and re-extracted the SDK, guix build is proceeding again
Did it work after re-extracting the SDK? I had no luck with 28.1. Trying 27.2 now.
💬 fjahr commented on pull request "doc: swap CPPFLAGS for APPEND_CPPFLAGS":
(https://github.com/bitcoin/bitcoin/pull/31819#issuecomment-2645930017)
ACK ea687d202934ee9aa26912cda21993da219cd418
Just used `APPEND_CPPFLAGS` yesterday for some debugging, so it works :)
(https://github.com/bitcoin/bitcoin/pull/31819#issuecomment-2645930017)
ACK ea687d202934ee9aa26912cda21993da219cd418
Just used `APPEND_CPPFLAGS` yesterday for some debugging, so it works :)
💬 luke-jr commented on issue "Duplicate coinbase transaction space reservation in CreateNewBlock":
(https://github.com/bitcoin/bitcoin/issues/21950#issuecomment-2645974712)
I suppose the ideal would be to create an actual generation transaction that fills the reserved weight/sigops, so the block validity check picks up on any error.
But blocks are already way too big, so I don't consider this a priority. Better to play it safe. The pennies unclaimed are nothing compared to the loss of an entire invalid block.
(https://github.com/bitcoin/bitcoin/issues/21950#issuecomment-2645974712)
I suppose the ideal would be to create an actual generation transaction that fills the reserved weight/sigops, so the block validity check picks up on any error.
But blocks are already way too big, so I don't consider this a priority. Better to play it safe. The pennies unclaimed are nothing compared to the loss of an entire invalid block.
🤔 rishkwal reviewed a pull request: "tests: Add witness commitment if we have a witness transaction in `FullBlockTest.update_block()`"
(https://github.com/bitcoin/bitcoin/pull/31823#pullrequestreview-2604068209)
Hi, looking at `feature_block.py` — it doesn't seem to contain any SegWit-specific validation tests. They remain in `feature_segwit.py`, `p2p_segwit.py`, etc; although I'm not completely confident about the scope of this file.
I have a couple of questions(avoid if irrelevant):
1. Is there any test case that can be covered in the current implementation that needs this change to be implemented?
2. Wouldn't it be better to create a new test file like `feature_<soft_fork>.py` to test the consensu
...
(https://github.com/bitcoin/bitcoin/pull/31823#pullrequestreview-2604068209)
Hi, looking at `feature_block.py` — it doesn't seem to contain any SegWit-specific validation tests. They remain in `feature_segwit.py`, `p2p_segwit.py`, etc; although I'm not completely confident about the scope of this file.
I have a couple of questions(avoid if irrelevant):
1. Is there any test case that can be covered in the current implementation that needs this change to be implemented?
2. Wouldn't it be better to create a new test file like `feature_<soft_fork>.py` to test the consensu
...
🤔 hodlinator reviewed a pull request: "qa: Fix `wallet_multiwallet.py`"
(https://github.com/bitcoin/bitcoin/pull/31410#pullrequestreview-2604068753)
Code review e717fb5a429d9d00e008fa1eb2b85179050be8fe
### Guix cross-build failure
Attempted to run Guix build e717fb5a429d9d00e008fa1eb2b85179050be8fe cross-built for Windows, but it fails already on test "0" / line 133:
https://github.com/bitcoin/bitcoin/blob/e717fb5a429d9d00e008fa1eb2b85179050be8fe/test/functional/wallet_multiwallet.py#L129-L133
<details><summary>Log</summary>
```
$ BITCOIND=/c/Users/hodlinator/Downloads/bitcoind-e717fb5a429d.exe build/test/functional/wallet_mu
...
(https://github.com/bitcoin/bitcoin/pull/31410#pullrequestreview-2604068753)
Code review e717fb5a429d9d00e008fa1eb2b85179050be8fe
### Guix cross-build failure
Attempted to run Guix build e717fb5a429d9d00e008fa1eb2b85179050be8fe cross-built for Windows, but it fails already on test "0" / line 133:
https://github.com/bitcoin/bitcoin/blob/e717fb5a429d9d00e008fa1eb2b85179050be8fe/test/functional/wallet_multiwallet.py#L129-L133
<details><summary>Log</summary>
```
$ BITCOIND=/c/Users/hodlinator/Downloads/bitcoind-e717fb5a429d.exe build/test/functional/wallet_mu
...
💬 onlinesipahimithu commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2646012270)
What is and how
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2646012270)
What is and how
🤔 onlinesipahimithu reviewed a pull request: "Add bitcoin-{node,gui} to release binaries for IPC"
(https://github.com/bitcoin/bitcoin/pull/31802#pullrequestreview-2604083934)
GOOD lesson from
(https://github.com/bitcoin/bitcoin/pull/31802#pullrequestreview-2604083934)
GOOD lesson from
💬 onlinesipahimithu commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#issuecomment-2646012743)
Helli how to develop
(https://github.com/bitcoin/bitcoin/pull/28710#issuecomment-2646012743)
Helli how to develop
⚠️ onlinesipahimithu opened an issue: "Hellobtc"
(https://github.com/bitcoin/bitcoin/issues/31828)
I trying for learn how works crypto
(https://github.com/bitcoin/bitcoin/issues/31828)
I trying for learn how works crypto
✅ pinheadmz closed an issue: "Hellobtc"
(https://github.com/bitcoin/bitcoin/issues/31828)
(https://github.com/bitcoin/bitcoin/issues/31828)
💬 eval-exec commented on pull request "Limit retries in GetRNDRRS to avoid infinite loop":
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1947992696)
Agree, I prefer fallback to `GetRNSR`.
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1947992696)
Agree, I prefer fallback to `GetRNSR`.
💬 eval-exec commented on pull request "Limit retries in GetRNDRRS to avoid infinite loop":
(https://github.com/bitcoin/bitcoin/pull/31826#issuecomment-2646035608)
> it would be better to instead investigate and fix the feature test or feature reporting.
Agree, it need time to invistigate this bug.
And I did some research, this CPU have bug on `rndrrs`:
> [04:28](https://oftc.irclog.whitequark.org/aarch64-laptops/2024-07-21#33392883) <HdkR> I found out yesterday that the bitcoin project uses rndrrs if it is available on aarch64 to fill its entropy pool. It spins in a loop until its pool is filled.
> [04:28](https://oftc.irclog.whitequark.org/aarch6
...
(https://github.com/bitcoin/bitcoin/pull/31826#issuecomment-2646035608)
> it would be better to instead investigate and fix the feature test or feature reporting.
Agree, it need time to invistigate this bug.
And I did some research, this CPU have bug on `rndrrs`:
> [04:28](https://oftc.irclog.whitequark.org/aarch64-laptops/2024-07-21#33392883) <HdkR> I found out yesterday that the bitcoin project uses rndrrs if it is available on aarch64 to fill its entropy pool. It spins in a loop until its pool is filled.
> [04:28](https://oftc.irclog.whitequark.org/aarch6
...
💬 eval-exec commented on pull request "Limit retries in GetRNDRRS to avoid infinite loop":
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1947993250)
Maybe should fallback to `GetRNDR`?
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1947993250)
Maybe should fallback to `GetRNDR`?
💬 sipa commented on pull request "Limit retries in GetRNDRRS to avoid infinite loop":
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1947994381)
I think it would be even better to do a trial run inside `InitHardwareRand`, so this condition can be detected, and the feature can be considered unsupported (so future calls to GetRNDRS will not even bother).
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1947994381)
I think it would be even better to do a trial run inside `InitHardwareRand`, so this condition can be detected, and the feature can be considered unsupported (so future calls to GetRNDRS will not even bother).
💬 theStack commented on pull request "contrib: fix `test_deterministic_coverage.sh` script for out-of-tree builds":
(https://github.com/bitcoin/bitcoin/pull/31588#issuecomment-2646119824)
@fjahr: Note that `GCOV_EXECUTABLE` is hardcoded in the script currently, so setting it as environment variable won't have any effect. Can you try again by modifying it directly in the file? It's not very user friendly that this is necessary, can add a commit that allows passing it via environment, if reviewers feel that this is still in scope of this PR.
(https://github.com/bitcoin/bitcoin/pull/31588#issuecomment-2646119824)
@fjahr: Note that `GCOV_EXECUTABLE` is hardcoded in the script currently, so setting it as environment variable won't have any effect. Can you try again by modifying it directly in the file? It's not very user friendly that this is necessary, can add a commit that allows passing it via environment, if reviewers feel that this is still in scope of this PR.