💬 achow101 commented on pull request "Silent Payments: receiving":
(https://github.com/bitcoin/bitcoin/pull/28202#discussion_r1288398953)
This looks in the UTXO set but is called on a transaction that has already been verified and so therefore it's inputs are no longer in the UTXO set. This results in the `Coin` being uninitialized which causes later `Solver` calls to fail unexpectedly.

💬 achow101 commented on pull request "Silent Payments: receiving":
(https://github.com/bitcoin/bitcoin/pull/28202#discussion_r1288396529)
Should recurse on `SCRIPTHASH` instead of assuming.

🤔 glozow reviewed a pull request: "net processing: clamp PeerManager::Options user input"
(https://github.com/bitcoin/bitcoin/pull/28149#pullrequestreview-1569427500)
reACK 547fa52443cbb5e8ccfee993486f5ced8cdbb33b

🚀 glozow merged a pull request: "net processing: clamp PeerManager::Options user input"
(https://github.com/bitcoin/bitcoin/pull/28149)

💬 fanquake commented on pull request "Break up script/standard.{h/cpp}":
(https://github.com/bitcoin/bitcoin/pull/28244#discussion_r1288431124)
```bash
src/script/solver.h seems to be missing the expected include guard:
#ifndef BITCOIN_SCRIPT_SOLVER_H
#define BITCOIN_SCRIPT_SOLVER_H
...
#endif // BITCOIN_SCRIPT_SOLVER_H
```

💬 achow101 commented on pull request "Silent Payments: receiving":
(https://github.com/bitcoin/bitcoin/pull/28202#discussion_r1288438010)
As discussed offline, we shouldn't use any view of the UTXO set. Rather we should use the undo data when receiving a `blockConnected`, and modify `TransactionAddedToMempool` to include the previous outputs. This lets us avoid any race conditions with the state of the UTXO set and guarantees that the previous output data will always be available for the silent payments calculations.

💬 theuni commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1671279124)
Yep, will do. I was just waiting for the others to settle.

🤔 glozow reviewed a pull request: "rpc: Add importmempool RPC"
(https://github.com/bitcoin/bitcoin/pull/27460#pullrequestreview-1569495734)
reACK fa776e61cd64a5ffd9a4be589ab8efeb5421861a

💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1288452570)
Ah! Might be worth being overly explanatory here.

💬 achow101 commented on pull request "Break up script/standard.{h/cpp}":
(https://github.com/bitcoin/bitcoin/pull/28244#discussion_r1288456318)
Done

💬 willcl-ark commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1288488390)
Seperated sanitzing checks and str->octal converstion in b4b0d2adc9

💬 willcl-ark commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1288489583)
b4b0d2adc9 now handles optional special mode bit

💬 willcl-ark commented on pull request "doc: Improve documentation of rpcallowip":
(https://github.com/bitcoin/bitcoin/pull/27480#discussion_r1288530860)
Took your suggestion here @pinheadmz

💬 willcl-ark commented on pull request "doc: Improve documentation of rpcallowip":
(https://github.com/bitcoin/bitcoin/pull/27480#issuecomment-1671383317)
> There hasn't been much activity lately. What is the status here?

Happy to close this, if we also close the corresponding issue #21070. But I think it's preferable to keep this open, and merge it to close the issue (and improve the docs).

💬 theuni commented on pull request "Use shared_ptr for CNode inside CConnman":
(https://github.com/bitcoin/bitcoin/pull/28222#issuecomment-1671392361)
@dergoegge Thanks for the ping.

> makes operations such as deleting them from multiple threads difficult

Why is this a goal?

I'm quite nervous about these changes. I've spoken with @dergoegge about this a few times now, but I'm afraid my information and opinions on the code involved are quite dated at this point (2017 to be exact :), so I haven't had much to contribute.

I'll say though, any changes or refactors for future changes to the threading model should be very well justified
...

💬 ryanofsky commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1288565564)
In commit "rpc: Add Arg() default helper" (fa56ad74160ba3c07a16a661fc14cded6deed9eb)

Haven't really looked at implementation yet, but can documentation be updated to say what happens with type mismatches? E.g. if arg is an int and you request a bool? Is the value just ignored and is a default returned instead? Is there an exception?

Also might be nice to add a short examples here showing what equivalent low level code is so developers can know how to use this helper and interpret it. Like
...

💬 dergoegge commented on pull request "Use shared_ptr for CNode inside CConnman":
(https://github.com/bitcoin/bitcoin/pull/28222#discussion_r1288400379)
Having the managed objects have a reference to the manager leads to absolute spaghetti code, so leaning towards NACK on that.

💬 dergoegge commented on pull request "Use shared_ptr for CNode inside CConnman":
(https://github.com/bitcoin/bitcoin/pull/28222#discussion_r1288527915)
You can just get rid of NodesSnapshot entirely now, it was only needed as a RAII helper for the manual ref counting.

💬 SambhavsCreation commented on issue "test: 999 of 999 multisig":
(https://github.com/bitcoin/bitcoin/issues/28179#issuecomment-1671550220)
Is someone still working on this? Please let me know as if not I might start working on it myself

💬 eriknylund commented on issue "test: 999 of 999 multisig":
(https://github.com/bitcoin/bitcoin/issues/28179#issuecomment-1671555701)
> Is someone still working on this? Please let me know as if not I might start working on it myself

I have a PR https://github.com/bitcoin/bitcoin/pull/28212 open for it, so please review if you have a chance. 🙏