💬 maflcko commented on pull request "Align legacy script policy with P2SH policy in AreInputsStandard":
(https://github.com/bitcoin/bitcoin/pull/33926#discussion_r2567485967)
@151henry151 I don't think it is a good use of anyone's time to ask an LLM to fundamentally change the script policy in a tight agent loop that uses the public repo resources to run and get the CI result continuously.

💬 ajtowns commented on pull request "Adds non-mempool wallet balance to overview":
(https://github.com/bitcoin-core/gui/pull/911#issuecomment-3584610611)
Screenshot:

<img width="318" height="218" alt="non-mempool-gui" src="https://github.com/user-attachments/assets/885411f4-d01a-4574-a341-98b962c5c2ec" />

The negative sign for the non-memool stuff isn't really very clear, perhaps.

✅ maflcko closed a pull request: "leveldb: correct trailer boundary check in Reader::SkipToInitialBlock"
(https://github.com/bitcoin/bitcoin/pull/33955)

💬 maflcko commented on pull request "leveldb: correct trailer boundary check in Reader::SkipToInitialBlock":
(https://github.com/bitcoin/bitcoin/pull/33955#issuecomment-3584610788)
thx, but this will need to go upstream

✅ maflcko closed a pull request: "Align legacy script policy with P2SH policy in AreInputsStandard"
(https://github.com/bitcoin/bitcoin/pull/33926)

💬 maflcko commented on pull request "Align legacy script policy with P2SH policy in AreInputsStandard":
(https://github.com/bitcoin/bitcoin/pull/33926#issuecomment-3584663392)
Closing, to break the bot loop

💬 151henry151 commented on pull request "Align legacy script policy with P2SH policy in AreInputsStandard":
(https://github.com/bitcoin/bitcoin/pull/33926#issuecomment-3584727791)
Sorry about that -- not trying to falsely represent anything here. I thought I had a pretty good start with my changes to policy.cpp, but then I've been struggling with getting the tests working. I know that overuse of LLMs is frowned upon and I realize that accidentally committing and pushing a bunch of MD files with notes generated by an LLM is not a good look, but I'm going to keep trying to figure this out locally and hope to make a valuable contribution here. I also realize that I should ha
...

💬 151henry151 commented on pull request "Align legacy script policy with P2SH policy in AreInputsStandard":
(https://github.com/bitcoin/bitcoin/pull/33926#discussion_r2567617676)
I didn't consider that I was burning through valuable resources, and I have certainly been using the LLM to help me troubleshoot and try to figure out why I can't get the tests to pass. Sorry for the abuse of resources and I'll use my own machine for tests until I get some good results.

💬 maflcko commented on pull request "test: check for output to stdout in `TestShell` test":
(https://github.com/bitcoin/bitcoin/pull/33951#issuecomment-3584759633)
lgtm ACK 52230a7f697fd99abdc4550d6a60737be024e246

💬 maflcko commented on pull request "qa: Remove no longer needed `feature_dirsymlinks.py`":
(https://github.com/bitcoin/bitcoin/pull/33924#issuecomment-3584772427)
Yeah, I guess there may not be a reduced path coverage inside Bitcoin Core right now, but will it always be that case in the future?

Is there some other benefit to removing the test?

👍 rkrux approved a pull request: "test: check for output to stdout in `TestShell` test"
(https://github.com/bitcoin/bitcoin/pull/33951#pullrequestreview-3514137749)
crACK 52230a7f697fd99abdc4550d6a60737be024e246

I didn't know about TestShell, looks quite handy!

💬 fanquake commented on pull request "depends: update freetype and document remaining `bitcoin-qt` runtime libs":
(https://github.com/bitcoin/bitcoin/pull/33952#issuecomment-3584839998)
Guix Build (aarch64):
```bash
97ee63f3335af4ef2fb32c5f99cf8d11c7f38a908f1ed9e202a11ecbde144db0 guix-build-50eb421d9efd/output/aarch64-linux-gnu/SHA256SUMS.part
e55dd97c02a5030fec0cac92f857159bd40820959dee8213df7f918d923cb714 guix-build-50eb421d9efd/output/aarch64-linux-gnu/bitcoin-50eb421d9efd-aarch64-linux-gnu-debug.tar.gz
0131ee5e1249d7295fbe0b601c401070b3d35a1a7c93b562ebb9324122608e88 guix-build-50eb421d9efd/output/aarch64-linux-gnu/bitcoin-50eb421d9efd-aarch64-linux-gnu.tar.gz
0f7326
...

💬 vasild commented on pull request "test: add functional test for outbound connection management":
(https://github.com/bitcoin/bitcoin/pull/33954#issuecomment-3584897920)
Concept ACK, thanks!

💬 kannapoix commented on pull request "test: Refactor rpc_getblockstats.py to use MiniWallet":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2567563419)
Do we need sync_all here? self.generate() already performs synchronization, and this test uses a single node. What are we syncing at this point?

💬 kannapoix commented on pull request "test: Refactor rpc_getblockstats.py to use MiniWallet":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2567706984)
Doesn’t removing these assertions degrade the test? The original test compared RPC results against predefined expected stats.

💬 kannapoix commented on pull request "test: Refactor rpc_getblockstats.py to use MiniWallet":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2567593821)
Testing that all stats share the same set of keys is different from testing that the required keys are present. To preserve the original test’s intent, should we hard-code the expected keys? Other tests follow this approach as well : https://github.com/bitcoin/bitcoin/blob/85d058dc537e62258ef3b3eb73589a242b885b8d/test/functional/rpc_blockchain.py#L135

💬 kannapoix commented on pull request "test: Refactor rpc_getblockstats.py to use MiniWallet":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2567726713)
Maybe these are added by mistake? Same lines are already exist above.

💬 kannapoix commented on pull request "test: Refactor rpc_getblockstats.py to use MiniWallet":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2567721695)
I think this also have a similar problem I pointed out before: https://github.com/bitcoin/bitcoin/pull/33184/files#r2567593821
This changes the test from testing against predefined expected data to cross-checking one getblockstats call with another (by hash). That’s a different test which may degrade the test.

🚀 fanquake merged a pull request: "test: check for output to stdout in `TestShell` test"
(https://github.com/bitcoin/bitcoin/pull/33951)

💬 maflcko commented on issue "node0 stderr bitcoind: validation.cpp:5392: void ChainstateManager::CheckBlockIndex() const: Assertion `!c->setBlockIndexCandidates.contains(const_cast<CBlockIndex*>(pindex))' failed.":
(https://github.com/bitcoin/bitcoin/issues/33948#issuecomment-3584926632)
It passed with `22~++20251126081852+97732ddb5d92-1~exp1`, fwiw.