💬 fanquake commented on pull request "Update libsecp256k1 subtree for 0.4.1 release":
(https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1875644508)
I would also prefer not to introduce some policy about only using tags, or similar, because we want to retain the ability to update the subtree to any commit we'd like, at any point.

👍 hebasto approved a pull request: "Update libsecp256k1 subtree for 0.4.1 release"
(https://github.com/bitcoin/bitcoin/pull/29169#pullrequestreview-1802641219)
ACK c13a17c6996442f04635bdf70ee8f06bf6854ff6, I've updated the `secp256k1` to its current master branch (efe85c70a2e357e3605a8901a9662295bae1001f) locally and got zero diff with this PR.

However, the PR title is a bit misleading as it mentions "0.4.1 release", which implies (at least for me) syncing to the `v0.4.1` tag.

👍 TheCharlatan approved a pull request: "doc: Rework guix docs after 1.4 release"
(https://github.com/bitcoin/bitcoin/pull/28962#pullrequestreview-1802673759)
ACK fad444f6e1b1ce5c1572c773db4a9a098c7a0b96

💬 brunoerg commented on pull request "PoC: fuzz chainstate and block managers":
(https://github.com/bitcoin/bitcoin/pull/29158#issuecomment-1875711592)
Nice one!

📝 fanquake opened a pull request: "contrib: add macho branch protection check"
(https://github.com/bitcoin/bitcoin/pull/29170)
Followup to https://github.com/bitcoin/bitcoin/pull/28459. Add a sanity check that `bti` instructions are present in the arm macho binary, similar to our x86_64 check for control flow.

Could do something similar for aarch64 linux in future, and maybe could use https://github.com/lief-project/LIEF/issues/975.

💬 TheCharlatan commented on pull request "PoC: fuzz chainstate and block managers":
(https://github.com/bitcoin/bitcoin/pull/29158#issuecomment-1875751255)
Concept ACK

💬 luke-jr commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1365807093)
As `nodeServices` is incorrect after this call, it seems to make more sense to just leave it passing `nLocalServices` here and the other call site?

💬 luke-jr commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1365836836)
Maybe it should be on by default?

💬 luke-jr commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1365833858)
No reason to delete the commas

💬 luke-jr commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1440760716)
Passing options individually like this is ugly - worse than just passing `const ArgsManager&` IMO. If you really want to cache it as a bool, maybe make a struct for connection permission options or something to pass instead.

Another possibility is to just cache a single `NetPermissionFlags` for these.

💬 sipa commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1440772571)
I think this comment should stay, because `NodeClock` isn't guaranteed to be monotonic?

💬 sr-gi commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#discussion_r1440804107)
Rebased and updated the comment. I also extended the test slightly to cover all possible cases

💬 achow101 commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1440702286)
In a9d67ad96e5f4d6dca64df4ac85f0434e17248ab "test: add tests for fundrawtx and sendmany rpcs"

There are existing tests for `sendmany` in wallet_basic.py. I think these tests could be moved there as well.

💬 achow101 commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1440697948)
In a9d67ad96e5f4d6dca64df4ac85f0434e17248ab "test: add tests for fundrawtx and sendmany rpcs"

nit: More pythonic to write:

```suggestion
tx.vout = [CTxOut(1 * COIN, bytearray(address_to_scriptpubkey(address)))] * 2
```

💬 furszy commented on pull request "wallet: Migrate entire address book entries to watchonly and solvables too":
(https://github.com/bitcoin/bitcoin/pull/28610#discussion_r1440800458)
Would be good to unload the wallets at the end of the test case.

👍 furszy approved a pull request: "wallet: Migrate entire address book entries to watchonly and solvables too"
(https://github.com/bitcoin/bitcoin/pull/28610#pullrequestreview-1802823938)
Code review ACK 406b71ab


With two non-blocking points:

Firstly, made a test to verify that the transaction's extra information is preserved during migration: c1aabd1b2a. This includes the preservation of bump fee info ('replaces_txid' and 'replaced_by_txid') and the user's custom comments ('comment' and 'comment_to'). Feel free to cherry-pick it, or I can push it in a follow-up. Either way is fine for me.

Secondly, shilling mode; with #26836, which now is part of #28574, this could ha
...

💬 furszy commented on pull request "wallet: Migrate entire address book entries to watchonly and solvables too":
(https://github.com/bitcoin/bitcoin/pull/28610#discussion_r1440735716)
tiny nit: could provide the `avoid_reuse` flag to `createwallet()` instead of calling `setwalletflag` separately.

💬 brunoerg commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1440838577)
Couldn't we fuzz ReadFlag with a name we didn't previously write?

💬 hebasto commented on issue "`-min` does not minimize wallet loading dialog":
(https://github.com/bitcoin-core/gui/issues/748#issuecomment-1875894079)
Confirming that the issue still exists for the release/Guix built `bitcoin-qt`.

> I guess something in the way we build qt is causing this?

The static Qt in depends misses GTK+ support. However, I'm not sure if it is the cause of the issue.

---

Other issues with different behavior for release binaries:
- https://github.com/bitcoin-core/gui/issues/33
- https://github.com/bitcoin-core/gui/issues/639

👍 jonasnick approved a pull request: "Update libsecp256k1 subtree for 0.4.1 release"
(https://github.com/bitcoin/bitcoin/pull/29169#pullrequestreview-1802996733)
ACK c13a17c6996442f04635bdf70ee8f06bf6854ff6 no difference to my locally checked out version.

📝 Christewart opened a pull request: "64bit arith"
(https://github.com/bitcoin/bitcoin/pull/29171)