💬 luke-jr commented on issue "build: document `BITCOIN_GENBUILD_NO_GIT` environment variable?":
(https://github.com/bitcoin/bitcoin/issues/31999#issuecomment-2702340828)
The intended use case is to avoid git trying to find a repo where there is none, which can trip security checks in some build systems.

💬 achow101 commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#issuecomment-2702517814)
As discussed at CoreDev, I've updated this PR to sync the best block periodically or when a connected block contains a relevant transaction.

I've also had to make a couple of changes to `AttachChain` following #31629

📝 suiyuan1314 opened a pull request: "docs: fix typos"
(https://github.com/bitcoin/bitcoin/pull/32006)

💬 pinheadmz commented on pull request "docs: fix typos":
(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.

💬 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.

💬 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

💬 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

💬 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?

💬 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
...

💬 mabu44 commented on pull request "[WIP] refactor: migrate unit tests to Google Test":
(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.

👍 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.

💬 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.

💬 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
```

💬 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.

💬 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.

🤔 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
...

💬 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?

💬 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.