💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1579531159)
@josibake can I get a helper function along the lines of:

```cpp
bool MaybeSilentPayment(CTransactionRef tx);
```

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2077270747)
@setavenger paging @josibake.

✅ achow101 closed an issue: "Calling `walletprocesspsbt` to sign multisig containing `OP_GREATERTHANOREQUAL` op_code"
(https://github.com/bitcoin/bitcoin/issues/29949)

💬 achow101 commented on issue "Calling `walletprocesspsbt` to sign multisig containing `OP_GREATERTHANOREQUAL` op_code":
(https://github.com/bitcoin/bitcoin/issues/29949#issuecomment-2077274021)
> Is there some other way to sign transaction, through bitcoind, which tries to spend from script which is not compatible with miniscript ?

No

> Is there some problem with `OP_GREATERTHANOREQUAL` which makes it non compatible with Miniscript ?

Yes, it is third party malleable, which makes it much harder to do analysis. For any additional signatures provided after the threshold has been reached, a third party can trivially remove those signatures and the transaction is still valid. Minis
...

💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1579549901)
I've done some more digging on this and looks like this is an interference between `test_orphan_consensus_failure` and `test_parent_consensus_failure`. The `low_fee_parent` created in the former is the exact same as the one created in the latter.

💬 maflcko commented on pull request "contrib: rpcauth.py - Add new option (-json) to output text in json format":
(https://github.com/bitcoin/bitcoin/pull/29433#issuecomment-2077307791)
ACK 9adf949d2aa6d199b85295b18c08967395b5570a

💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1579554883)
> this is from `test_orphan_consensus_failure` causing the exact same parent to be added to `m_recent_rejects_reconsiderable`
>
> Can we somehow cause chaintip updates during cleanup, without dropping minfee too much? What if cleanup detects that it went down too much, then calls `fill_mempool` again?

lol, I missed this 🤣

💬 instagibbs commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1579558091)
Adding this block to `cleanup` resets the cache, though I haven't looked at if this messes with minfee too much:

```
# Cause small re-org and tickle AlreadyHaveTx to flush reject filters between tests
chaintip = self.nodes[0].getbestblockhash()
self.nodes[0].invalidateblock(chaintip)
peer_sender = self.nodes[0].add_p2p_connection(P2PInterface())
peer_sender.send_message(msg_tx(CTransaction()))
peer_sender.wait_for_d
...

💬 setavenger commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2077317678)
I don't think extracting the public keys is the issue as they are pretty standard. So I think it has to be the input_hash. I don't know C++ but I suspect that the linked part does (txid, vout_as_int) and compares the txid and then the vouts but as integers. The transaction in question has 4 outpoints for the lowest txid. And the vouts are 136, 202, 204 and 383. The LEs are 0x88000000, 0xca000000, 0xcc000000 and 0x7f010000. Lexicographically 0x7f010000<0x88000000 but sorted as vout one gets 136 <
...

👍 stickies-v approved a pull request: "refactor: Use our own implementation of urlDecode"
(https://github.com/bitcoin/bitcoin/pull/29904#pullrequestreview-2022657131)
ACK 992c714451676cee33d3dff49f36329423270c1c

nit: definitely not worth more back-and-forth, but I think (untested) maflcko's [suggestion](https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1578145369) should work with MSVC when explicitly converting iterators to pointers:

<details>
<summary>git diff on 992c714451</summary>

```diff
diff --git a/src/common/url.cpp b/src/common/url.cpp
index ecf88d07ea..c901ecb5dc 100644
--- a/src/common/url.cpp
+++ b/src/common/url.cpp
@@ -14
...

💬 fjahr commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1579563791)
thanks, I will add them when I have to retouch, if not I will make a small follow-up

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1579565183)
> Can we somehow cause chaintip updates during cleanup, without dropping minfee too much? What if cleanup detects that it went down too much, then calls fill_mempool again?

Hm, `fill_mempool` isn't really built for this (though we can change that). I'll try to see if we can make sure we don't reuse utxos.

💬 alfonsoromanz commented on pull request "depends: pass verbose through to cmake based makefiles":
(https://github.com/bitcoin/bitcoin/pull/29960#issuecomment-2077329585)
Concept ACK

💬 fjahr commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#issuecomment-2077331441)
> I think (untested) maflcko's [suggestion](https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1578145369) should work with MSVC when explicitly converting iterators to pointers

Yes, it's certainly possible and I briefly explored it myself, but it didn't seem like much of an improvement over the current version anymore. And the current version had already received some review at that point, so I decided it would be better to just revert since it was just a style-nit anyway.

🤔 furszy reviewed a pull request: "Disable util::Result copying and assignment"
(https://github.com/bitcoin/bitcoin/pull/29906#pullrequestreview-2022455655)
ACK 6602231564

💬 furszy commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1579500335)
Not for this PR but.. we could remove this `Update` call pretty easily (e8a330584571bf0270c692676a57ebcf7d7c7697). We are calling `getNewDestination` twice only to verify if the wallet is locked or not.

💬 furszy commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1579447042)
q: why uppercase? Do you wanted to differentiate own functions vs standard ones?

👍 willcl-ark approved a pull request: "test: Add test case for spending bare multisig"
(https://github.com/bitcoin/bitcoin/pull/29120#pullrequestreview-2022689116)
ACK 9c7b2d0d37b31f01403f7a0f2ea25b60b126c841

Confirmed this tests that when the `-permitbaremultisig=0` policy is set, the mempool still permits spending (confirmed) bare multisig outputs. Mempool rejection of this output type is already tested earlier in the same test file.

👍 willcl-ark approved a pull request: "contrib: rpcauth.py - Add new option (-json) to output text in json format"
(https://github.com/bitcoin/bitcoin/pull/29433#pullrequestreview-2022714587)
tACK 9adf949d2aa6d199b85295b18c08967395b5570a

Makes sense to me to be able to output this in JSON for automated setups.

👍 ryanofsky approved a pull request: "[refactor] Check CTxMemPool options in ctor"
(https://github.com/bitcoin/bitcoin/pull/28830#pullrequestreview-2022700917)
Code review ACK 30a1024b63105a97d04149e83ae2d8cf0830cf69

> Wasn't sure what `std::error& error` referred to, so just chose `util::Error`, but would be easy enough to use something else.

Sorry, that was just a confusing typo. I mean to write `std::string& error`. But I guess that doesn't work because the error message is actually translated. In that case, I do think `bilingual& error` would be better than `util::Error& error` just to make code simpler and more obvious. But no strong opinion
...