👍 darosior approved a pull request: "test: check tx is final when there is no locktime"
(https://github.com/bitcoin/bitcoin/pull/33030#pullrequestreview-3042985591)
utACK 065e42976a70738770af256da810ddc1316a4496

💬 darosior commented on pull request "test: check tx is final when there is no locktime":
(https://github.com/bitcoin/bitcoin/pull/33030#discussion_r2222521762)
nit, don't retouch this for me, but my point with the comment wasn't to describe the line below (anyone can just read block height is 0 and locktime is 0) but to describe *why* them both being 0 is a special case worth exercising, as you did in OP.

💬 naiyoma commented on pull request "test: add test cases to wallet_signer.py":
(https://github.com/bitcoin/bitcoin/pull/33020#discussion_r2222554679)
done https://github.com/bitcoin/bitcoin/commit/38a6293fa89104f3d6f1907d04c2e4c488157307

💬 naiyoma commented on pull request "test: add test cases to wallet_signer.py":
(https://github.com/bitcoin/bitcoin/pull/33020#issuecomment-3102747234)

> For the third commit [74fb47f](https://github.com/bitcoin/bitcoin/commit/74fb47f071451145b550062e382416db9388433d): I suggest deleting the commented out code. It's not doing anything useful for the test, and I can't remember why I wrote it.

@Sjors, thanks for the review.
I have also deleted this part https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_signer.py#L235

💬 Sjors commented on pull request "test: add test cases to wallet_signer.py":
(https://github.com/bitcoin/bitcoin/pull/33020#issuecomment-3102748924)
ACK da318fe53fa954cc3eadd7c39e17eb3da7c3e09e

💬 stickies-v commented on pull request "p2p: rename GetAddresses -> GetAddressesUnsafe":
(https://github.com/bitcoin/bitcoin/pull/32994#issuecomment-3102815266)
re-ACK 1cb23997033c395d9ecd7bf2f54787b134485f41

💬 maflcko commented on pull request "test: functional test for incomplete PSBT with additional signatures required":
(https://github.com/bitcoin/bitcoin/pull/33035#issuecomment-3102843496)
@marshallshen is the code llm generated?

💬 fanquake commented on pull request "Update secp256k1 subtree to latest master":
(https://github.com/bitcoin/bitcoin/pull/33036#issuecomment-3102874210)
Guix Build (x86_64 & aarch64):
```bash
483e252d112c73c25ef86b8f9ba2bd2acd27b4209a55dd2b0c819badaa7e5123 guix-build-336b8be37b22/output/aarch64-linux-gnu/SHA256SUMS.part
53c2f3879c967deb4706cdf12bf54798a7ef4d3bf54701b6695abe5cd50c7435 guix-build-336b8be37b22/output/aarch64-linux-gnu/bitcoin-336b8be37b22-aarch64-linux-gnu-debug.tar.gz
818f15221bd362f772b9b7c4e07842dbf6965678eb1dd0c86b8d15031b725731 guix-build-336b8be37b22/output/aarch64-linux-gnu/bitcoin-336b8be37b22-aarch64-linux-gnu.tar.g
...

💬 stickies-v commented on pull request "Update secp256k1 subtree to latest master":
(https://github.com/bitcoin/bitcoin/pull/33036#issuecomment-3102989198)
I think the 0.7.0 release has been updated to fix wrong 0.7.1 references, I'm getting a slight diff when pulling the subtree. Current HEAD for https://github.com/bitcoin-core/secp256k1/releases/tag/v0.7.0 is https://github.com/bitcoin-core/secp256k1/commit/a660a4976efe880bae7982ee410b9e0dc59ac983, whereas this PR is using https://github.com/bitcoin-core/secp256k1/commit/b9313c6e1a6082a66b4c75777e18ca4b176fcf9d with [diff](https://github.com/bitcoin-core/secp256k1/compare/b9313c6e1a6082a66b4c7577
...

💬 fanquake commented on pull request "Update secp256k1 subtree to latest master":
(https://github.com/bitcoin/bitcoin/pull/33036#issuecomment-3103000113)
> whereas this PR is using https://github.com/bitcoin-core/secp256k1/commit/b9313c6e1a6082a66b4c75777e18ca4b176fcf9d

Yes that is what I say in the PR description.

💬 murchandamus commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2222739458)
Isn’t that sufficient to demonstrate that it is doing that?

💬 marshallshen commented on pull request "test: functional test for incomplete PSBT with additional signatures required":
(https://github.com/bitcoin/bitcoin/pull/33035#issuecomment-3103097218)
@kevkevinpal - thanks! I will use it to ensure it does increase the test coverage.
@maflcko - I used LLM to help me understand the codebase and write parts of the test, my workflow is:
- understand structure of the TestFramework
- have a goal of what to test (in this case, PSBT is incomplete if some signature is missing)
- reusing the existing test artifacts, and try to write the test to match my goal above

Please let me know if there's anything I am missing as I'm still new to the codeba
...

👍 stickies-v approved a pull request: "Update secp256k1 subtree to latest master"
(https://github.com/bitcoin/bitcoin/pull/33036#pullrequestreview-3043431053)
> Yes that is what I say in the PR description.

Sorry, the link to the 0.7.0 release notes led me to believe the intention was to upgrade to the latest release instead of master, but I now understand (and as indicated in the PR title) that's not the usual update process used for secp256k1.

---

ACK 336b8be37b2260d8e902b93f1761265a0aefa496

Verified that the subtree pull matches https://github.com/bitcoin-core/secp256k1/commit/b9313c6e1a6082a66b4c75777e18ca4b176fcf9d, and briefly skimme
...

👍 theStack approved a pull request: "test: add test cases to wallet_signer.py"
(https://github.com/bitcoin/bitcoin/pull/33020#pullrequestreview-3043484317)
ACK da318fe53fa954cc3eadd7c39e17eb3da7c3e09e

nit: could update the PR title to also indicate that also (commented out) tests get removed

🤔 marcofleon reviewed a pull request: "[29.x] Backport #32521"
(https://github.com/bitcoin/bitcoin/pull/33013#pullrequestreview-3043558663)
ACK 716b4362c0b93dc666464088ed81742c4abf7732

Made sure the changes here match https://github.com/bitcoin/bitcoin/pull/32521. All unit and functional tests pass on 29.x.

💬 enirox001 commented on pull request "test: check tx is final when there is no locktime":
(https://github.com/bitcoin/bitcoin/pull/33030#issuecomment-3103441542)
ACK 065e429: Valuable test case that explicitly demonstrates `IsFinalTx` behavior when nLockTime is 0, including at genesis block height.

💬 Sjors commented on pull request "Update secp256k1 subtree to latest master":
(https://github.com/bitcoin/bitcoin/pull/33036#issuecomment-3103533914)
We have in the past updated to release tags, see e.g. #31216. Though that seems mostly a matter of how you phrase the commit title.

📝 frankomosh opened a pull request: "fuzz: add mempool_dag fuzzer for transaction dependency testing"
(https://github.com/bitcoin/bitcoin/pull/33038)
adds a new target, `mempool_dag`, to test mempool DAG invariants by constructing transaction chains with controlled parent/child dependencies. main focus areas include:

a. ancestor / descendant count and size limits (`-limitancestorcount,` `-limitancestorsize`, `-limitdescendantcount`, `-limitdescendantsize`)

b. TRUC-policy interaction with non-TRUC transactions

c. mempool eviction behaviour under shrinking `-maxmempool`

d. fee-rate consistency after partial package eviction

Inva
...

🤔 darosior reviewed a pull request: "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts"
(https://github.com/bitcoin/bitcoin/pull/32473#pullrequestreview-3038560723)
I've been playing with this code for the past couple of days and this is looking good. I wrote a unit test which sanity checks the caching and notably exercises false positive cache hits, which appears to not currently be covered (see LINK): https://github.com/darosior/bitcoin/commit/f303c6d68b5b9b3b23af34c8ef9efa7822fbe38b. I ran IBD on mainnet and signet with caching for blocks enabled and it did not result in any historical block being rejected on either of these networks. I'll do testnet3 an
...

💬 darosior commented on pull request "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts":
(https://github.com/bitcoin/bitcoin/pull/32473#discussion_r2219495270)
nit: no need to special-case when `kwargs` is empty since `ctx == {**ctx, **{}}`.
```suggestion
return lambda ctx: get({**ctx, **kwargs}, name)
```