💬 pinheadmz commented on pull request "docs: fix typos":
(https://github.com/bitcoin/bitcoin/pull/32006#discussion_r1982598597)
I think "thorough" is correct here
(https://github.com/bitcoin/bitcoin/pull/32006#discussion_r1982598597)
I think "thorough" is correct here
💬 laanwj commented on issue "guix: re-enable exported symbol checking for RISC-V":
(https://github.com/bitcoin/bitcoin/issues/28095#issuecomment-2702668461)
Yes, thanks for the reminder.
(https://github.com/bitcoin/bitcoin/issues/28095#issuecomment-2702668461)
Yes, thanks for the reminder.
💬 suiyuan1314 commented on pull request "docs: fix typos":
(https://github.com/bitcoin/bitcoin/pull/32006#discussion_r1982617243)
yes. i guess it means pass all the testing cases. sorry for the inconvenience. i have reverted this change.
(https://github.com/bitcoin/bitcoin/pull/32006#discussion_r1982617243)
yes. i guess it means pass all the testing cases. sorry for the inconvenience. i have reverted this change.
💬 achow101 commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1982741716)
If I need to retouch
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1982741716)
If I need to retouch
💬 achow101 commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1982741759)
If I need to retouch
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1982741759)
If I need to retouch
💬 fanquake commented on issue "build: document `BITCOIN_GENBUILD_NO_GIT` environment variable?":
(https://github.com/bitcoin/bitcoin/issues/31999#issuecomment-2703142176)
> in some build systems
Which build systems?
(https://github.com/bitcoin/bitcoin/issues/31999#issuecomment-2703142176)
> in some build systems
Which build systems?
💬 mabu44 commented on pull request "[WIP] refactor: migrate unit tests to Google Test":
(https://github.com/bitcoin/bitcoin/pull/31988#issuecomment-2703240658)
> (This PR was meant as a POC. Should probably be a draft and/or just opened in personal repo).
Yes, this was a draft PR before being closed.
> My impression is that the way we use CTest+Boost.Test does output currently running tests, so this is no longer an issue.
Right, but still valid if test_bitcoin is run directly, for instance because you need to provide custom parameters or execute only specific tests.
> This seems like a deal-breaker unfortunately, but still interesting to s
...
(https://github.com/bitcoin/bitcoin/pull/31988#issuecomment-2703240658)
> (This PR was meant as a POC. Should probably be a draft and/or just opened in personal repo).
Yes, this was a draft PR before being closed.
> My impression is that the way we use CTest+Boost.Test does output currently running tests, so this is no longer an issue.
Right, but still valid if test_bitcoin is run directly, for instance because you need to provide custom parameters or execute only specific tests.
> This seems like a deal-breaker unfortunately, but still interesting to s
...
💬 mabu44 commented on pull request "[WIP] refactor: migrate unit tests to Google Test":
(https://github.com/bitcoin/bitcoin/pull/31988#discussion_r1982983516)
Yes
(https://github.com/bitcoin/bitcoin/pull/31988#discussion_r1982983516)
Yes
💬 mabu44 commented on pull request "[WIP] refactor: migrate unit tests to Google Test":
(https://github.com/bitcoin/bitcoin/pull/31988#discussion_r1982991466)
I started doing this kind of changes to increase clarity, but then realized that these improvements can be difficult to automate using scripted-diffs. So I avoided doing that on other tests.
(https://github.com/bitcoin/bitcoin/pull/31988#discussion_r1982991466)
I started doing this kind of changes to increase clarity, but then realized that these improvements can be difficult to automate using scripted-diffs. So I avoided doing that on other tests.
👍 rkrux approved a pull request: "wallet: fix crash on double block disconnection"
(https://github.com/bitcoin/bitcoin/pull/31757#pullrequestreview-2663779609)
ACK 11f8ab140fe63857f6a93b81021efda8f90ceeda
Thank for you adding verbose comments in the test, specially because of the peculiar flow involved in reproducing the bug.
(https://github.com/bitcoin/bitcoin/pull/31757#pullrequestreview-2663779609)
ACK 11f8ab140fe63857f6a93b81021efda8f90ceeda
Thank for you adding verbose comments in the test, specially because of the peculiar flow involved in reproducing the bug.
💬 rkrux commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1982956981)
> This issue blocks any future spending and results in an incorrect balance display.
Technically correct but since the block was supposed to be invalidated and with the assumption that the likelihood for it to be invalidated again (successfully this time) is high, this immature unspent will not be blocked for long.
Not suggesting any changes, just noteworthy. Please do LMK if my assumption is incorrect.
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1982956981)
> This issue blocks any future spending and results in an incorrect balance display.
Technically correct but since the block was supposed to be invalidated and with the assumption that the likelihood for it to be invalidated again (successfully this time) is high, this immature unspent will not be blocked for long.
Not suggesting any changes, just noteworthy. Please do LMK if my assumption is incorrect.
💬 rkrux commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1982978353)
This comment is too positive. If I the node undergoes through an unclean shutdown and restart here, the wallet crashes again.
The underlying issue of the discrepancy between wallet-view and chainstate-view is still there that causes the failure.
Can update the comment to the following but I don't intend to block the PR just for this.
```
Ensure wallet state is consistent for now if the node doesn't undergo an unclean shutdown
```
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1982978353)
This comment is too positive. If I the node undergoes through an unclean shutdown and restart here, the wallet crashes again.
The underlying issue of the discrepancy between wallet-view and chainstate-view is still there that causes the failure.
Can update the comment to the following but I don't intend to block the PR just for this.
```
Ensure wallet state is consistent for now if the node doesn't undergo an unclean shutdown
```
💬 rkrux commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1982979160)
+1 for adding this to wrap the test up.
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1982979160)
+1 for adding this to wrap the test up.
💬 rkrux commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1982994302)
I would have used node RPCs here because these don't seem to be wallet specific requests and moreover, there's just one loaded wallet and using wallet RPCs here made me think there was some specific reason for it, but fine. I don't want the ACKs to be invalidated for this.
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1982994302)
I would have used node RPCs here because these don't seem to be wallet specific requests and moreover, there's just one loaded wallet and using wallet RPCs here made me think there was some specific reason for it, but fine. I don't want the ACKs to be invalidated for this.
🤔 hodlinator reviewed a pull request: "miniscript: fixes #29098 by only use first k valid signatures"
(https://github.com/bitcoin/bitcoin/pull/31719#pullrequestreview-2663700357)
Hi @tnndbtc,
Thank you for looking into this issue! Great to have someone more comfortable than me at touching more sensitive parts of the code. Leaving some feedback from my brief testing.
I think it would be good if this PR did one of either:
a) Directly included 387c12e0813a863bc9728777719acbafe7b12a34 from #28212, or
b) Not mention 387c12e0813a863bc9728777719acbafe7b12a34/#28212 as much as it is doing - instead focusing on #29098.
Having a timeout is more palpable than relying on
...
(https://github.com/bitcoin/bitcoin/pull/31719#pullrequestreview-2663700357)
Hi @tnndbtc,
Thank you for looking into this issue! Great to have someone more comfortable than me at touching more sensitive parts of the code. Leaving some feedback from my brief testing.
I think it would be good if this PR did one of either:
a) Directly included 387c12e0813a863bc9728777719acbafe7b12a34 from #28212, or
b) Not mention 387c12e0813a863bc9728777719acbafe7b12a34/#28212 as much as it is doing - instead focusing on #29098.
Having a timeout is more palpable than relying on
...
💬 hodlinator commented on pull request "miniscript: fixes #29098 by only use first k valid signatures":
(https://github.com/bitcoin/bitcoin/pull/31719#discussion_r1982912213)
1) Since the first commit causes the fuzzer to fail, and the approach in the project is that no commit should introduce errors, I think it makes more sense to squash the 2 commits into 1.
2) Please avoid inserting the loop in the middle of a pre-existing 2-line comment. Could you also add a comment describing why the added loop was necessary?
(https://github.com/bitcoin/bitcoin/pull/31719#discussion_r1982912213)
1) Since the first commit causes the fuzzer to fail, and the approach in the project is that no commit should introduce errors, I think it makes more sense to squash the 2 commits into 1.
2) Please avoid inserting the loop in the middle of a pre-existing 2-line comment. Could you also add a comment describing why the added loop was necessary?
💬 hodlinator commented on pull request "miniscript: fixes #29098 by only use first k valid signatures":
(https://github.com/bitcoin/bitcoin/pull/31719#discussion_r1982914577)
I think it would be more useful to add comments like this directly with the code, rather than in PR comments.
(https://github.com/bitcoin/bitcoin/pull/31719#discussion_r1982914577)
I think it would be more useful to add comments like this directly with the code, rather than in PR comments.
💬 mabu44 commented on pull request "[WIP] refactor: migrate unit tests to Google Test":
(https://github.com/bitcoin/bitcoin/pull/31988#discussion_r1982998479)
This instruction is the reason I needed Google Mock. Otherwise, I could have avoided such a dependency in the POC and checked with ASSERT_EQ on size and on items.
(https://github.com/bitcoin/bitcoin/pull/31988#discussion_r1982998479)
This instruction is the reason I needed Google Mock. Otherwise, I could have avoided such a dependency in the POC and checked with ASSERT_EQ on size and on items.
💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2703309315)
Or could pass an additional argument to `waitNext()`: `CThreadInterrupt` and have `waitNext()` honor that one in addition to `chainman().m_interrupt`.
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2703309315)
Or could pass an additional argument to `waitNext()`: `CThreadInterrupt` and have `waitNext()` honor that one in addition to `chainman().m_interrupt`.
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2703359479)
The problem is a bit less urgent now, since I refactored the Template Provider to have one thread per connection. Worst case this thread lingers for half an hour until a new block arrives. It only becomes a potential DoS problem for public facing template providers, but that use case needs additional safety and performance changes anyway.
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2703359479)
The problem is a bit less urgent now, since I refactored the Template Provider to have one thread per connection. Worst case this thread lingers for half an hour until a new block arrives. It only becomes a potential DoS problem for public facing template providers, but that use case needs additional safety and performance changes anyway.