📝 stickies-v opened a pull request: "RPC: access RPC arguments by name"
(https://github.com/bitcoin/bitcoin/pull/29277)
Adds string overloads for the `RPCHelpMan::Arg` and `RPCHelpMan::MaybeArg` helpers to be able to access RPC arguments by name instead of index number. Especially in RPCs with a large number of parameters, this can be quite helpful.
Example usage:
```cpp
const auto action{self.Arg<std::string>("action")};
```
Most of the LoC is adding test coverage and documentation updates. No behaviour change.
An alternative approach to #27788 with significantly less overhaul.
(https://github.com/bitcoin/bitcoin/pull/29277)
Adds string overloads for the `RPCHelpMan::Arg` and `RPCHelpMan::MaybeArg` helpers to be able to access RPC arguments by name instead of index number. Especially in RPCs with a large number of parameters, this can be quite helpful.
Example usage:
```cpp
const auto action{self.Arg<std::string>("action")};
```
Most of the LoC is adding test coverage and documentation updates. No behaviour change.
An alternative approach to #27788 with significantly less overhaul.
💬 tromp commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1899207755)
> 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.
Bitcoin script is just a simple programming language with specific technical rules that all programs must follow. On top of that there are additional technical rules that programs must follow to fall in certain categories, like isStandard.
None of these rules can express int
...
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1899207755)
> 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.
Bitcoin script is just a simple programming language with specific technical rules that all programs must follow. On top of that there are additional technical rules that programs must follow to fall in certain categories, like isStandard.
None of these rules can express int
...
💬 maflcko commented on pull request "rpc: Be able to access RPC parameters by name":
(https://github.com/bitcoin/bitcoin/pull/27788#issuecomment-1899208902)
Can be closed after https://github.com/bitcoin/bitcoin/pull/29277 ?
(https://github.com/bitcoin/bitcoin/pull/27788#issuecomment-1899208902)
Can be closed after https://github.com/bitcoin/bitcoin/pull/29277 ?
🤔 furszy reviewed a pull request: "Avoid returning references to mutex guarded members"
(https://github.com/bitcoin/bitcoin/pull/28774#pullrequestreview-1830559572)
ACK 32a9f13c
(https://github.com/bitcoin/bitcoin/pull/28774#pullrequestreview-1830559572)
ACK 32a9f13c
💬 furszy commented on pull request "Avoid returning references to mutex guarded members":
(https://github.com/bitcoin/bitcoin/pull/28774#discussion_r1457979323)
nit:
Could inline it
```c++
return WITH_LOCK(cs_wallet, return cb(vMasterKey));
```
Also, could pass `cb` as a const ref.
(https://github.com/bitcoin/bitcoin/pull/28774#discussion_r1457979323)
nit:
Could inline it
```c++
return WITH_LOCK(cs_wallet, return cb(vMasterKey));
```
Also, could pass `cb` as a const ref.
💬 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());
```
(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
(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
(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)
(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`?
(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
...
(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)
(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...
(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.
(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.
(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
(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
(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
(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
...
(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?
(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?