💬 stickies-v commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1277584643)
Would it make sense to rename this to `emplace`, to keep the interface in line with `optional` and `expected`?

💬 sr-gi commented on pull request "Improves addnode / m_added_nodes logic":
(https://github.com/bitcoin/bitcoin/pull/28155#issuecomment-1656076583)
Added a test to `test/functional/rpc_net.py` and fixed the check on `CConnman::AddNode`

👍 jamesob approved a pull request: "Rework validation logic for assumeutxo"
(https://github.com/bitcoin/bitcoin/pull/27746#pullrequestreview-1552660659)
reACK a733dd79e29068ad1e0532ac42a45188a040a7b9 ([`jamesob/ackr/27746.5.sdaftuar.rework_validation_logic`](https://github.com/jamesob/bitcoin/tree/ackr/27746.5.sdaftuar.rework_validation_logic))

Changes incorporate feedback discussed above.


<details><summary>Show signature data</summary>
<p>

```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

reACK a733dd79e29068ad1e0532ac42a45188a040a7b9 ([`jamesob/ackr/27746.5.sdaftuar.rework_validation_logic`](https://github.com/jamesob/bitcoi
...

💬 kevkevinpal commented on pull request "Improves addnode / m_added_nodes logic":
(https://github.com/bitcoin/bitcoin/pull/28155#discussion_r1277945499)
might want to uncomment these other tests

💬 sr-gi commented on pull request "Improves addnode / m_added_nodes logic":
(https://github.com/bitcoin/bitcoin/pull/28155#discussion_r1277960418)
Ups, my bad

💬 ariard commented on pull request "CONTRIBUTING: Caution against using AI/LLMs (ChatGPT, Copilot, etc)":
(https://github.com/bitcoin/bitcoin/pull/28175#issuecomment-1656240907)
Sent a mail to Jess from the Bitcoin Defense Legal Fund to collect more legal opinions with the context. Normally luke-jr (luke@dashjr.org) and jonatack (jon@atack.com) are cc.

💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1656279094)
> https://duckduckgo.com/?q=chattr+inside+docker should fix it

We're still having trouble getting this to work:
https://github.com/achow101/bitcoin/commits/28171-chattr-fixes
https://cirrus-ci.com/build/5095863295410176

🤔 Sjors reviewed a pull request: "Rework validation logic for assumeutxo"
(https://github.com/bitcoin/bitcoin/pull/27746#pullrequestreview-1552855193)
Also reviewed a733dd79e29068ad1e0532ac42a45188a040a7b9, d4a11abb1972b54f0babdccfbb2fde97ab885933, 3556b850221bc0e597d7dd749d4d47ab58dc8083 and read through all of `CheckBlockIndex()`. Will continue later (going mostly in reverse order).

💬 Sjors commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1278023724)
3556b850221bc0e597d7dd749d4d47ab58dc8083: if you have to retouch, can you add a comment that `!CBlockIndexWorkComparator()(pindex, c->m_chain.Tip())` means "pindex has more work than the tip or was received earlier"

💬 Sjors commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1278035016)
3556b850221bc0e597d7dd749d4d47ab58dc8083: there's a tie breaker in the comparator for if two blocks with equal work were loaded from disk. Presumably that only matters if you restart right during a reorg in some weird way.

💬 pinheadmz commented on pull request "net: Add new permission `forceinbound` to evict a random unprotected connection if all slots are otherwise full":
(https://github.com/bitcoin/bitcoin/pull/27600#issuecomment-1656289233)
@naumenkogs I added one more commit that limits the number of simultaneous forced-inbound connections to 8. I didn't add it as an init option yet but we could if you like that approach. A connection only counts as forced-inbound if it succeeded in evicting another peer, and that is marked by a new bool in `CNodeOptions`

💬 achow101 commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1656327982)
ACK 19b6353d3866769ea84b7759396078745c0a8ecf

Although the `chattr` approach worked locally, we couldn't figure out how to make it work in CI, so the approach of running CI as a non-root user seems reasonable.

💬 achow101 commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1278078208)
> It is an undocumented requirement that the functional usdt tests are run as root (not sure why).

For whatever reason, USDTs don't work for non-root users.

💬 luke-jr commented on pull request "CONTRIBUTING: Caution against using AI/LLMs (ChatGPT, Copilot, etc)":
(https://github.com/bitcoin/bitcoin/pull/28175#discussion_r1278081971)
I mentioned these specifically because:

1. ChatGPT is the most popularly known, and most likely to be searched for if someone is considering using it.
2. GitHub promotes use of Copilot heavily, and we are using GitHub.
3. Meta is falsely advertising LLaMA as open source, and many people are just believing that without verifying. (The source code is not available, and the license is not permissive)

💬 achow101 commented on pull request "script: check op_verif and op_vernotif":
(https://github.com/bitcoin/bitcoin/pull/28169#issuecomment-1656430144)
Concept ACK

I agree that `BAD_OPCODE` is the wrong error for these opcodes. However, instead of adding a new opcode, I think these should just return the existing `DISABLED_OPCODE` error that is used for the other disabled opcodes.

Also `OP_VER` should be included here as well as that's really what is disabled. `OP_VERIF` and `OP_VERNOTIF` are really just variants of `OP_VER`.

💬 Dearfor commented on pull request "script: check op_verif and op_vernotif":
(https://github.com/bitcoin/bitcoin/pull/28169#issuecomment-1656471607)
Ok thanks.

🤔 furszy reviewed a pull request: "wallet: bugfix, disallow migration of invalid scripts"
(https://github.com/bitcoin/bitcoin/pull/28125#pullrequestreview-1553154514)
Feedback tackled, thanks achow101.

Expanded test coverage with the requested label check and few more checks. Now the test:
1) Verifies that neither the invalid double sh script label, nor the `addr(<script_hash_addr>)` descriptor, are contained by any of the migrated wallets.
2) Verifies that the valid sh script (the original single sh script that is imported at the same time as the invalid one) and its address book record are contained by the migrated watch-only wallet.

Also, while was
...

💬 furszy commented on pull request "wallet: bugfix, disallow migration of invalid scripts":
(https://github.com/bitcoin/bitcoin/pull/28125#discussion_r1278210618)
> Perhaps check that this label doesn't appear in any of the migrated wallets?

Sure, expanded the test to cover it, and also added few other additional cases.

💬 ariard commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1278215639)
So if my understanding of the API is correct, you receive a package from `AcceptPackage()`, and there is a call to the constructor `AncestorPackage()`, then we verify each transaction being component of the package is in the mempool by wtxid or txid or `PreChecks()` them.

If the transaction is `PreChecks()` valid, then we call `AddFeeAndVsize()`. If we have a `TX_SINGLE_FAILURE`, we add `AddFeeAndVsize()` or if we have a `TX_MISSING_INPUTS`, we `SkipWithDescendants()` the transaction. All oth
...

💬 hebasto commented on pull request "ci: Run Windows native task on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28173#issuecomment-1656668537)
> In the meantime it may be good to do 10 runs and then check how many of them fail

Observing intermittent "Error: no RPC connection". Converting this PR to a draft for now