💬 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?
💬 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).
(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
...
(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.
(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
...
(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.
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1899406273)
Things you've noted are addressed by the availability of OP_RETURN.
💬 stickies-v commented on pull request "RPC: access RPC arguments by name":
(https://github.com/bitcoin/bitcoin/pull/29277#issuecomment-1899415885)
Force pushed to make CI happy:
- removed doxygen @params since it didn't pick up the overloads (and it's already documented in plain-text anyway)
- added a null return type to the testing `RPCHelpMan`
(https://github.com/bitcoin/bitcoin/pull/29277#issuecomment-1899415885)
Force pushed to make CI happy:
- removed doxygen @params since it didn't pick up the overloads (and it's already documented in plain-text anyway)
- added a null return type to the testing `RPCHelpMan`
📝 theStack opened a pull request: "test: p2p: check disconnect due to lack of desirable service flags"
(https://github.com/bitcoin/bitcoin/pull/29279)
This PR adds missing test coverage for disconnecting peers which don't offer the desirable service flags in their VERSION message:
https://github.com/bitcoin/bitcoin/blob/5f3a0574c45477288bc678b15f24940486084576/src/net_processing.cpp#L3384-L3389
This check is relevant for the connection types "outbound-full-relay", "block-relay-only" and "addr-fetch" (see `CNode::ExpectServicesFromConn(...)`). Feeler connections always disconnect, which is also tested here.
In lack of finding a proper file
...
(https://github.com/bitcoin/bitcoin/pull/29279)
This PR adds missing test coverage for disconnecting peers which don't offer the desirable service flags in their VERSION message:
https://github.com/bitcoin/bitcoin/blob/5f3a0574c45477288bc678b15f24940486084576/src/net_processing.cpp#L3384-L3389
This check is relevant for the connection types "outbound-full-relay", "block-relay-only" and "addr-fetch" (see `CNode::ExpectServicesFromConn(...)`). Feeler connections always disconnect, which is also tested here.
In lack of finding a proper file
...
📝 reardencode opened a pull request: "Implement OP_CHECKTEMPLATEVERIFY"
(https://github.com/bitcoin/bitcoin/pull/29280)
This pull request forks from #29198 and only includes the implementation of OP_CHECKTEMPLATEVERIFY, its tests (except the functional ones), and standardization of bare OP_CHECKTEMPLATEVERIFY locking scripts.
(https://github.com/bitcoin/bitcoin/pull/29280)
This pull request forks from #29198 and only includes the implementation of OP_CHECKTEMPLATEVERIFY, its tests (except the functional ones), and standardization of bare OP_CHECKTEMPLATEVERIFY locking scripts.
💬 ajtowns commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-1899825975)
> Agree, but the idea here is not to discard log messages, the idea just is to attach meaningful metadata to log messages so they can be filtered by component.
Filtering is discarding.
If you just want to *find* log messages, that what's grep is for, and if you want to make it more fine-grained than "hey this is an important log message/warning/error", that's what `-logsourcelocations` is for.
If it's any help, I think it's better to think of the sections as not related to which section
...
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-1899825975)
> Agree, but the idea here is not to discard log messages, the idea just is to attach meaningful metadata to log messages so they can be filtered by component.
Filtering is discarding.
If you just want to *find* log messages, that what's grep is for, and if you want to make it more fine-grained than "hey this is an important log message/warning/error", that's what `-logsourcelocations` is for.
If it's any help, I think it's better to think of the sections as not related to which section
...
💬 Apdlrcjafg19 commented on issue "Libbitcoinkernel Project Tracking":
(https://github.com/bitcoin/bitcoin/issues/27587#issuecomment-1899940946)
> #### Project Board: https://github.com/orgs/bitcoin/projects/3
> This is the tracking issue for the `libbitcoinkernel` project. The original tracking issue is found in [#24303](https://github.com/bitcoin/bitcoin/issues/24303). This issue contains much of the content written by Carl Dong in the original issue but is more regularly updated.
>
> The libbitcoinkernel project is a new attempt at extracting our consensus engine. The kernel part of the name highlights one of the key functional diffe
...
(https://github.com/bitcoin/bitcoin/issues/27587#issuecomment-1899940946)
> #### Project Board: https://github.com/orgs/bitcoin/projects/3
> This is the tracking issue for the `libbitcoinkernel` project. The original tracking issue is found in [#24303](https://github.com/bitcoin/bitcoin/issues/24303). This issue contains much of the content written by Carl Dong in the original issue but is more regularly updated.
>
> The libbitcoinkernel project is a new attempt at extracting our consensus engine. The kernel part of the name highlights one of the key functional diffe
...
💬 ktecho commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1899954524)
> Which of these [opcodes](https://en.bitcoin.it/wiki/Script#Opcodes) is not working as implemented or [documented](https://github.com/ChrisCho-H/bitcoin/blob/0bf55345764d22c5653c0a374647aa5582a186a2/src/script/script.h)?
It's not a single opcode that is not working as implemented or documented. It's the whole system that is not behaving as expected.
If you have an API so users of your site can upload pictures for blog posts, and you found that somebody uploaded a 35 GB blueray-ripped movi
...
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1899954524)
> Which of these [opcodes](https://en.bitcoin.it/wiki/Script#Opcodes) is not working as implemented or [documented](https://github.com/ChrisCho-H/bitcoin/blob/0bf55345764d22c5653c0a374647aa5582a186a2/src/script/script.h)?
It's not a single opcode that is not working as implemented or documented. It's the whole system that is not behaving as expected.
If you have an API so users of your site can upload pictures for blog posts, and you found that somebody uploaded a 35 GB blueray-ripped movi
...
💬 1ma commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1899975514)
> > tACK
>
> You can add the commit that you tested after tACK: [8672721](https://github.com/bitcoin/bitcoin/commit/8672721deb06e66854a085c9994e99c99160b8a1)
>
> Or more details what exactly was tested and reasons to agree with pull request.
I already gave my reasons back in August, top of the thread. This isn't Twitter, if you want to participate here make an effort to read everything through an through.
As for what was tested, obviously the full PR. I built @Retropex's branch on my
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1899975514)
> > tACK
>
> You can add the commit that you tested after tACK: [8672721](https://github.com/bitcoin/bitcoin/commit/8672721deb06e66854a085c9994e99c99160b8a1)
>
> Or more details what exactly was tested and reasons to agree with pull request.
I already gave my reasons back in August, top of the thread. This isn't Twitter, if you want to participate here make an effort to read everything through an through.
As for what was tested, obviously the full PR. I built @Retropex's branch on my
...