💬 stickies-v commented on pull request "refactor: remove CTxMemPool::queryHashes()":
(https://github.com/bitcoin/bitcoin/pull/29260#discussion_r1457983060)
Sorry, I misunderstood the test. Replaced now with:

```cpp
const auto new_size{WITH_LOCK(tx_pool.cs, return tx_pool.entryAll().size())};
assert(new_size < info_all.size());
```

💬 ismaelsadeeq commented on pull request "RPC: access RPC arguments by name":
(https://github.com/bitcoin/bitcoin/pull/29277#issuecomment-1899213515)
Concept ACK

💬 stickies-v commented on pull request "refactor: remove CTxMemPool::queryHashes()":
(https://github.com/bitcoin/bitcoin/pull/29260#issuecomment-1899222086)
Force pushed to re-instate the fuzz test, addressing https://github.com/bitcoin/bitcoin/pull/29260#discussion_r1457572196

💬 stickies-v commented on pull request "refactor: remove CTxMemPool::queryHashes()":
(https://github.com/bitcoin/bitcoin/pull/29260#discussion_r1457991155)
`entryAll` returning just refs has less overhead than `infoAll` so I think regardless this is a preferred approach? (even if it's not a huge difference)

💬 dergoegge commented on pull request "refactor: remove CTxMemPool::queryHashes()":
(https://github.com/bitcoin/bitcoin/pull/29260#discussion_r1457992730)
could use `infoAll` to avoid the `WITH_LOCK`?

💬 achow101 commented on issue "macOS App Notarization & Stapling":
(https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-1899228781)
Did some tests on MacOS 13.5.1. There is no difference in phone home behavior for no notarization at all, notarization without stapling, and stapled notarization. Tested with Bitcoin Core 26.0, Wireshark (notarized but not stapled), and Firefox (notarized and stapled), and did packet capture for each with Wireshark. They all resulted in communication with api.apple-cloudkit.com, which, AFAIK, is the domain for the notarization check.

To replicate this experiment, the command `spctl -a -vvv -t
...

✅ achow101 closed a pull request: "rpc: Be able to access RPC parameters by name"
(https://github.com/bitcoin/bitcoin/pull/27788)

💬 dergoegge commented on pull request "refactor: remove CTxMemPool::queryHashes()":
(https://github.com/bitcoin/bitcoin/pull/29260#discussion_r1457997458)
That overhead is tiny if any, I'd think. You can benchmark MempoolToJSON and it'd bet it won't even show up. But anyway we can't use it due to the sequence sync requirement.

Actually, maybe it's worth considering removing `entryAll` too? it's only used in two places and we can work around the sync issue somehow, but not in this PR...

💬 murchandamus commented on pull request "wallet: fix coin selection tracing to return -1 when no change pos":
(https://github.com/bitcoin/bitcoin/pull/29272#issuecomment-1899270783)
Returning `-1` if there is no change seems reasonable to me, but it looks like it’s cast to an unsigned int somewhere else in the stack. I’m not familiar with the tracing framework interna, so it’s not obvious to me how to fix this.

💬 stickies-v commented on pull request "refactor: remove CTxMemPool::queryHashes()":
(https://github.com/bitcoin/bitcoin/pull/29260#discussion_r1458018860)
Looks like there's a much simpler `tx_pool.size()` option too, using that now.

👍 dergoegge approved a pull request: "refactor: remove CTxMemPool::queryHashes()"
(https://github.com/bitcoin/bitcoin/pull/29260#pullrequestreview-1830623478)
Code review ACK 282b12ddb0aeb1f0991dd9f45c6b5c5c079652ec

💬 achow101 commented on pull request "wallet: fix coin selection tracing to return -1 when no change pos":
(https://github.com/bitcoin/bitcoin/pull/29272#issuecomment-1899294026)
ACK 8c6d266aa61ffc409b50ece4e649f7a74ec7acb7

💬 1ma commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1899307301)
tACK

📝 ismaelsadeeq opened a pull request: "RPC: Wallet: Add `maxfeerate` and `maxburnamount` startup option and use `maxfeerate` to check max wallet tx feerate"
(https://github.com/bitcoin/bitcoin/pull/29278)
This PR attempts to fix two issues

#29217
#29220

Firstly,
- Both `sendrawtransaction` and `testmempoolaccept` have the optional parameter `maxfeerate`.
When passed, this parameter ensures that any transactions with a fee rate higher than the specified `maxfeerate` value are rejected.

- `maxburnamount` is also an optional parameter for `sendrawtransaction`. When passed, it rejects transactions with unspendable outputs (e.g., 'datacarrier' outputs using the OP_RETURN opcode) greater t
...

💬 1440000bytes commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1899323441)
> Again, let's be clear: Cybersecurity 101 stuff here. The ability to inject arbitrary data into any system in a way that isn't explicitly documented as permitted for an expected purpose of the system is always a vulnerability and always something that should be addressed.

You can use nlocktime and amounts in outputs to decode it to some text. Will that need a CVE ID?

💬 remyers commented on pull request "wallet: fix coin selection tracing to return -1 when no change pos":
(https://github.com/bitcoin/bitcoin/pull/29272#discussion_r1458048472)
I had my casting on the wrong value - we need to cast the unsigned optional value. Fixed in [e701982](https://github.com/bitcoin/bitcoin/pull/29272/commits/e701982c741edd2182690ddf4628e4c079a5185e).

💬 wizkid057 commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1899379741)
> > Again, let's be clear: Cybersecurity 101 stuff here. The ability to inject arbitrary data into any system in a way that isn't explicitly documented as permitted for an expected purpose of the system is always a vulnerability and always something that should be addressed.
>
> You can use nlocktime and amounts in outputs to decode it to some text. Will that need a CVE ID?

You're clearly using a bit of hyperbole here, again in yet another attempt at a "gotcha" moment without addressing _a
...

💬 1440000bytes commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1899384781)
> tACK

You can add the commit that you tested after tACK: https://github.com/bitcoin/bitcoin/pull/28217/commits/8672721deb06e66854a085c9994e99c99160b8a1

Or more details what exactly was tested and reasons to agree with pull request.

💬 1440000bytes commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1899396269)
> Storing data in nLockTime is not happening as it's not really even exploitable. 32-bits max, but far less in practice, since would need to be a value permitted to be included in a block... so maybe 27 bits or so max per transaction? Just use OP_RETURN. Cheaper and more efficient.

https://x.com/1440000bytes/status/1732580146203250731

https://bitcoin.stackexchange.com/questions/23792/can-someone-explain-nlocktime/

> Store data in amounts is convoluted since you'd need a significant amou
...

💬 wizkid057 commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1899406273)
Things you've noted are addressed by the availability of OP_RETURN.