💬 rot13maxi commented on pull request "CAT in Tapscript (BIP-347)":
(https://github.com/bitcoin/bitcoin/pull/29247#discussion_r1757499656)
`SCRIPT_VERIFY_DISCOURAGE_OP_CAT` is acting as a CAT-specific `SCRIPT_VERIFY_DISCOURAGE_OP_SUCCESS` that can be turned off while still not relaying transasctions with other OP_SUCCESS opcodes. Since this is the first time we're carving out an OP_SUCCESS and doing this, it would be great to have a test that exercises the combinations of SCRIPT_VERIFY_DISCOURAGE_OP_CAT and SCRIPT_VERIFY_OP_CAT
(https://github.com/bitcoin/bitcoin/pull/29247#discussion_r1757499656)
`SCRIPT_VERIFY_DISCOURAGE_OP_CAT` is acting as a CAT-specific `SCRIPT_VERIFY_DISCOURAGE_OP_SUCCESS` that can be turned off while still not relaying transasctions with other OP_SUCCESS opcodes. Since this is the first time we're carving out an OP_SUCCESS and doing this, it would be great to have a test that exercises the combinations of SCRIPT_VERIFY_DISCOURAGE_OP_CAT and SCRIPT_VERIFY_OP_CAT
💬 l0rinc commented on pull request "build: optimize .h generation in GenerateHeaderFrom{Raw,Json}.cmake":
(https://github.com/bitcoin/bitcoin/pull/30888#discussion_r1757506396)
I thought of that but couldn't find a way to do it inline - and defing a new variable in a separate line just for this makes it less readable
```cmake
string(REPEAT "." 16 line_width)
string(REGEX REPLACE "${line_width}" "\\0\n" formatted_bytes "${hex_content}")
```
(https://github.com/bitcoin/bitcoin/pull/30888#discussion_r1757506396)
I thought of that but couldn't find a way to do it inline - and defing a new variable in a separate line just for this makes it less readable
```cmake
string(REPEAT "." 16 line_width)
string(REGEX REPLACE "${line_width}" "\\0\n" formatted_bytes "${hex_content}")
```
💬 l0rinc commented on pull request "build: optimize .h generation in GenerateHeaderFrom{Raw,Json}.cmake":
(https://github.com/bitcoin/bitcoin/pull/30888#discussion_r1757521241)
Done, thanks
(https://github.com/bitcoin/bitcoin/pull/30888#discussion_r1757521241)
Done, thanks
💬 mzumsande commented on issue "Race condition between ZMQ UpdateTip and getblocktemplate":
(https://github.com/bitcoin/bitcoin/issues/30862#issuecomment-2347161468)
can you rule out that someone else on the network just mined a block, and the inconclusive `getblocktemplate` result belongs to a previous call that was sent off before that new block was connected?
(https://github.com/bitcoin/bitcoin/issues/30862#issuecomment-2347161468)
can you rule out that someone else on the network just mined a block, and the inconclusive `getblocktemplate` result belongs to a previous call that was sent off before that new block was connected?
💬 fjahr commented on pull request "scripted-diff: Modernize nLocalServices to m_local_services":
(https://github.com/bitcoin/bitcoin/pull/30885#discussion_r1757532379)
Hm, ok, I interpreted the comment on the namespace above as that we ignore that here. But I missed the variable below and I the others are just older. Changed.
(https://github.com/bitcoin/bitcoin/pull/30885#discussion_r1757532379)
Hm, ok, I interpreted the comment on the namespace above as that we ignore that here. But I missed the variable below and I the others are just older. Changed.
💬 davidgumberg commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#issuecomment-2347208216)
I think that there are also some calls to `std::rewind`:
https://github.com/bitcoin/bitcoin/blob/cf0120ff024aa73a56f2975c832fda6aa8146dfa/src/test/streams_tests.cpp#L264
and `std::fgetc`:
https://github.com/bitcoin/bitcoin/blob/cf0120ff024aa73a56f2975c832fda6aa8146dfa/src/node/utxo_snapshot.cpp#L76
that are problematic if we are caching file position.
Also, if it seems sensible to mitigate the risk of someone modifying the file position in the future without changing `m_position`
...
(https://github.com/bitcoin/bitcoin/pull/30884#issuecomment-2347208216)
I think that there are also some calls to `std::rewind`:
https://github.com/bitcoin/bitcoin/blob/cf0120ff024aa73a56f2975c832fda6aa8146dfa/src/test/streams_tests.cpp#L264
and `std::fgetc`:
https://github.com/bitcoin/bitcoin/blob/cf0120ff024aa73a56f2975c832fda6aa8146dfa/src/node/utxo_snapshot.cpp#L76
that are problematic if we are caching file position.
Also, if it seems sensible to mitigate the risk of someone modifying the file position in the future without changing `m_position`
...
🤔 furszy reviewed a pull request: "interfaces: #30697 follow ups"
(https://github.com/bitcoin/bitcoin/pull/30828#pullrequestreview-2301395429)
Code review ACK 84663291275248fd52da644b0c2566bbf9cc780b
It seems that with the latest changes, the `deleteRwSettings` is no longer used.
It would be nice to add test coverage for it and for the null value usage as well.
(https://github.com/bitcoin/bitcoin/pull/30828#pullrequestreview-2301395429)
Code review ACK 84663291275248fd52da644b0c2566bbf9cc780b
It seems that with the latest changes, the `deleteRwSettings` is no longer used.
It would be nice to add test coverage for it and for the null value usage as well.
👍 marcofleon approved a pull request: "refactor: move m_is_inbound out of CNodeState"
(https://github.com/bitcoin/bitcoin/pull/30233#pullrequestreview-2301402833)
ACK 07f4cebe5780f1039541d989e64b70eccc5b4eb5
I took a look at c308e3200d6968be553ed031d09b4bccb46b504b and 2e0b6742b82e60ea685afd25f2d19b8b558678ce in https://github.com/bitcoin/bitcoin/pull/30116 (assuming those commits are mostly final) to understand a bit better how `m_is_inbound` will be used for Erlay. Based on that, I'd say the changes in this PR make sense.
(https://github.com/bitcoin/bitcoin/pull/30233#pullrequestreview-2301402833)
ACK 07f4cebe5780f1039541d989e64b70eccc5b4eb5
I took a look at c308e3200d6968be553ed031d09b4bccb46b504b and 2e0b6742b82e60ea685afd25f2d19b8b558678ce in https://github.com/bitcoin/bitcoin/pull/30116 (assuming those commits are mostly final) to understand a bit better how `m_is_inbound` will be used for Erlay. Based on that, I'd say the changes in this PR make sense.
🤔 furszy reviewed a pull request: "scripted-diff: Modernize nLocalServices to m_local_services"
(https://github.com/bitcoin/bitcoin/pull/30885#pullrequestreview-2301405397)
utACK 33381ea530ad79ac1e04c37f5707e93d3e0509ca
(https://github.com/bitcoin/bitcoin/pull/30885#pullrequestreview-2301405397)
utACK 33381ea530ad79ac1e04c37f5707e93d3e0509ca
💬 furszy commented on pull request "scripted-diff: Modernize nLocalServices to m_local_services":
(https://github.com/bitcoin/bitcoin/pull/30885#discussion_r1757575935)
tiny nit:
I know this is an scripted-diff but.. could call `GetLocalServices()` here.
(https://github.com/bitcoin/bitcoin/pull/30885#discussion_r1757575935)
tiny nit:
I know this is an scripted-diff but.. could call `GetLocalServices()` here.
💬 mcelrath commented on issue "Race condition between ZMQ UpdateTip and getblocktemplate":
(https://github.com/bitcoin/bitcoin/issues/30862#issuecomment-2347235405)
I believe that's answered positively by the timestamps and structure of the
calling code.
A better test would be to reproduce this with regtest which I'll create a
script to demonstrate soon.
But at the same time it seems there should be a mutex around the chain tip.
If there isn't this is basically guaranteed to happen. I haven't looked yet.
On Thu, Sep 12, 2024, 4:18 PM Martin Zumsande ***@***.***>
wrote:
> can you rule out that someone else on the network just mined a block, a
...
(https://github.com/bitcoin/bitcoin/issues/30862#issuecomment-2347235405)
I believe that's answered positively by the timestamps and structure of the
calling code.
A better test would be to reproduce this with regtest which I'll create a
script to demonstrate soon.
But at the same time it seems there should be a mutex around the chain tip.
If there isn't this is basically guaranteed to happen. I haven't looked yet.
On Thu, Sep 12, 2024, 4:18 PM Martin Zumsande ***@***.***>
wrote:
> can you rule out that someone else on the network just mined a block, a
...
💬 katesalazar commented on issue "Closing a wallet using the fa46088440 28.x QT client segfaults":
(https://github.com/bitcoin/bitcoin/issues/30887#issuecomment-2347237302)
In my environment, 5d15485a is a crashing rev
and inmediate ancestor 1a41e635 is a non-crashing rev.
(https://github.com/bitcoin/bitcoin/issues/30887#issuecomment-2347237302)
In my environment, 5d15485a is a crashing rev
and inmediate ancestor 1a41e635 is a non-crashing rev.
🤔 jonatack reviewed a pull request: "scripted-diff: Modernize nLocalServices to m_local_services"
(https://github.com/bitcoin/bitcoin/pull/30885#pullrequestreview-2301419943)
Review ACK 33381ea530ad79ac1e04c37f5707e93d3e0509ca
Following the last push, suggest renaming the pull title to `scripted-diff: Modernize nLocalServices naming`
(https://github.com/bitcoin/bitcoin/pull/30885#pullrequestreview-2301419943)
Review ACK 33381ea530ad79ac1e04c37f5707e93d3e0509ca
Following the last push, suggest renaming the pull title to `scripted-diff: Modernize nLocalServices naming`
💬 katesalazar commented on issue "Closing a wallet using the fa46088440 28.x QT client segfaults":
(https://github.com/bitcoin/bitcoin/issues/30887#issuecomment-2347251385)
I came back to fa46088440, then reverted 5d15485a on top, _looks like healed_.
(https://github.com/bitcoin/bitcoin/issues/30887#issuecomment-2347251385)
I came back to fa46088440, then reverted 5d15485a on top, _looks like healed_.
💬 fjahr commented on pull request "scripted-diff: Modernize nLocalServices naming":
(https://github.com/bitcoin/bitcoin/pull/30885#issuecomment-2347258648)
> Following the last push, suggest renaming the pull title to `scripted-diff: Modernize nLocalServices naming` (edit: and also the commit name)
done
(https://github.com/bitcoin/bitcoin/pull/30885#issuecomment-2347258648)
> Following the last push, suggest renaming the pull title to `scripted-diff: Modernize nLocalServices naming` (edit: and also the commit name)
done
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2347263134)
Rebased.
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2347263134)
Rebased.
💬 furszy commented on issue "Closing a wallet using the fa46088440 28.x QT client segfaults":
(https://github.com/bitcoin/bitcoin/issues/30887#issuecomment-2347263442)
Replicated. It happens closing it through the GUI toolbar button. Not the command-line. Will investigate it.
(https://github.com/bitcoin/bitcoin/issues/30887#issuecomment-2347263442)
Replicated. It happens closing it through the GUI toolbar button. Not the command-line. Will investigate it.
🤔 ryanofsky reviewed a pull request: "multiprocess: Add IPC wrapper for Mining interface"
(https://github.com/bitcoin/bitcoin/pull/30510#pullrequestreview-2301289042)
Updated 1be749c771cd9fd80361ebb69c87482920b25cd1 -> b95bb2179610183d9398d50d8c8fd24b1450ad4d ([`pr/mine-types.10`](https://github.com/ryanofsky/bitcoin/commits/pr/mine-types.10) -> [`pr/mine-types.11`](https://github.com/ryanofsky/bitcoin/commits/pr/mine-types.11), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine-types.10..pr/mine-types.11)) switching to concepts instead of enable_if
(https://github.com/bitcoin/bitcoin/pull/30510#pullrequestreview-2301289042)
Updated 1be749c771cd9fd80361ebb69c87482920b25cd1 -> b95bb2179610183d9398d50d8c8fd24b1450ad4d ([`pr/mine-types.10`](https://github.com/ryanofsky/bitcoin/commits/pr/mine-types.10) -> [`pr/mine-types.11`](https://github.com/ryanofsky/bitcoin/commits/pr/mine-types.11), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine-types.10..pr/mine-types.11)) switching to concepts instead of enable_if
💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1757602140)
re: https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1757372497
Removed enable_if so this should be simpler now. Thanks for bringing this up!
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1757602140)
re: https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1757372497
Removed enable_if so this should be simpler now. Thanks for bringing this up!
📝 fjahr opened a pull request: "test: Check already deactivated network stays suspended after dumptxoutset"
(https://github.com/bitcoin/bitcoin/pull/30892)
Follow-up to #30817 which covered the robustness of `dumptxoutset`: network is deactivated during the run but re-activated even when an issue was encountered. But it did not cover the case if the user had deactivated the network themselves before. In that case the user may want the network to stay off so the network is not reactivated after `dumptxoutset` finishes. A test for this behavior is added here.
(https://github.com/bitcoin/bitcoin/pull/30892)
Follow-up to #30817 which covered the robustness of `dumptxoutset`: network is deactivated during the run but re-activated even when an issue was encountered. But it did not cover the case if the user had deactivated the network themselves before. In that case the user may want the network to stay off so the network is not reactivated after `dumptxoutset` finishes. A test for this behavior is added here.