💬 marcofleon commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1750661750)
You're right, fixed. This also made me realize that I don't really need the loop. But I'm thinking to do a followup where it would build on the chain of 16 headers, as opposed to just sending 16 headers one time and resetting. So I think I'll keep the loop there for now, as it shouldn't make a difference as far as I'm aware.
(https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1750661750)
You're right, fixed. This also made me realize that I don't really need the loop. But I'm thinking to do a followup where it would build on the chain of 16 headers, as opposed to just sending 16 headers one time and resetting. So I think I'll keep the loop there for now, as it shouldn't make a difference as far as I'm aware.
💬 marcofleon commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1750674964)
With REGTEST enabled, it's blocked here:
https://github.com/bitcoin/bitcoin/blob/712a2b5453cdf2568fece94b969d6e0923b6ba16/src/validation.cpp#L4204-L4209
Using MAIN params it passes because those soft forks aren't deployed and so the header is added to the block index and you'll get the failure. I could add `header.nVersion = 4` in the `ConsumeHeader` function of the harness to make it pass for both main and regtest. What do you think?
(https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1750674964)
With REGTEST enabled, it's blocked here:
https://github.com/bitcoin/bitcoin/blob/712a2b5453cdf2568fece94b969d6e0923b6ba16/src/validation.cpp#L4204-L4209
Using MAIN params it passes because those soft forks aren't deployed and so the header is added to the block index and you'll get the failure. I could add `header.nVersion = 4` in the `ConsumeHeader` function of the harness to make it pass for both main and regtest. What do you think?
💬 marcofleon commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1750676867)
Good suggestion, thank you.
(https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1750676867)
Good suggestion, thank you.
💬 instagibbs commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1750677345)
Let the fuzzer choose the version, I think?
(https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1750677345)
Let the fuzzer choose the version, I think?
👍 itornaza approved a pull request: "multiprocess: Add -ipcbind option to bitcoin-node"
(https://github.com/bitcoin/bitcoin/pull/30509#pullrequestreview-2290623740)
Code review ACK 30073e6b3a24cbe417c45cd5df6a3a2de0251e9d
Followed step by step the implementation through the code and especially the `src/ipc/*` changed files and it looks solid. Great that it provides yet another step to facilitate the stratum v2 integration into core.
(https://github.com/bitcoin/bitcoin/pull/30509#pullrequestreview-2290623740)
Code review ACK 30073e6b3a24cbe417c45cd5df6a3a2de0251e9d
Followed step by step the implementation through the code and especially the `src/ipc/*` changed files and it looks solid. Great that it provides yet another step to facilitate the stratum v2 integration into core.
✅ ouziel-slama closed an issue: "No ZMQ `sequence` notifications on Regtest"
(https://github.com/bitcoin/bitcoin/issues/30853)
(https://github.com/bitcoin/bitcoin/issues/30853)
💬 ouziel-slama commented on issue "No ZMQ `sequence` notifications on Regtest":
(https://github.com/bitcoin/bitcoin/issues/30853#issuecomment-2338810297)
Thanks for the link! actually it works fine, the error comes from elsewhere! sorry for the false alarm!
(https://github.com/bitcoin/bitcoin/issues/30853#issuecomment-2338810297)
Thanks for the link! actually it works fine, the error comes from elsewhere! sorry for the false alarm!
💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2338890606)
Added 1 commits 083b886bc6b98fab53e2c6b7dc99a017daa9726e -> 3749b730171e93a0beb3dcc458d440ee7035b3b9 ([`pr/mine-types.5`](https://github.com/ryanofsky/bitcoin/commits/pr/mine-types.5) -> [`pr/mine-types.6`](https://github.com/ryanofsky/bitcoin/commits/pr/mine-types.6), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine-types.5...pr/mine-types.6)) testing https://github.com/chaincodelabs/libmultiprocess/pull/110 fix to add explicit cmake generated file dependency and fix unreliable pa
...
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2338890606)
Added 1 commits 083b886bc6b98fab53e2c6b7dc99a017daa9726e -> 3749b730171e93a0beb3dcc458d440ee7035b3b9 ([`pr/mine-types.5`](https://github.com/ryanofsky/bitcoin/commits/pr/mine-types.5) -> [`pr/mine-types.6`](https://github.com/ryanofsky/bitcoin/commits/pr/mine-types.6), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine-types.5...pr/mine-types.6)) testing https://github.com/chaincodelabs/libmultiprocess/pull/110 fix to add explicit cmake generated file dependency and fix unreliable pa
...
💬 l0rinc commented on pull request "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750804998)
Dropped the commit after the suggested simplification
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750804998)
Dropped the commit after the suggested simplification
💬 l0rinc commented on pull request "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750805360)
> I'm not sure I see benefit of changing this from unsigned char to uint8_t
`uint8_t` was already used in the other branches
> changing uint8_t generally to value_type
I like that, thanks!
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750805360)
> I'm not sure I see benefit of changing this from unsigned char to uint8_t
`uint8_t` was already used in the other branches
> changing uint8_t generally to value_type
I like that, thanks!
💬 l0rinc commented on pull request "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750805634)
Thanks
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750805634)
Thanks
💬 l0rinc commented on pull request "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750806256)
Dropped these after your simplification suggestion
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750806256)
Dropped these after your simplification suggestion
💬 l0rinc commented on pull request "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750811705)
Based on what I read C++23 solves some of these problems (e.g. `Every specialization of std::span is a TriviallyCopyable type. (since C++23)`) and while array and vector casts would likely be safe, thanks for pointing out that the layout may not always be the same and we need something more predictable here.
Wanted to avoid explicit pointer arithmetic or extra copying, let me know if the latest push is a good compromise in this area.
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750811705)
Based on what I read C++23 solves some of these problems (e.g. `Every specialization of std::span is a TriviallyCopyable type. (since C++23)`) and while array and vector casts would likely be safe, thanks for pointing out that the layout may not always be the same and we need something more predictable here.
Wanted to avoid explicit pointer arithmetic or extra copying, let me know if the latest push is a good compromise in this area.
💬 l0rinc commented on pull request "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750812828)
Because we were casting back and forth this way. But I think I found a good enough generalization with your help, we don't need to connect the two operators this way.
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750812828)
Because we were casting back and forth this way. But I think I found a good enough generalization with your help, we don't need to connect the two operators this way.
💬 l0rinc commented on pull request "test: add subsidy sum test, iterating every block":
(https://github.com/bitcoin/bitcoin/pull/30699#issuecomment-2338961152)
@fjahr, @brunoerg, please reevaluate the concept NACKs. I'm no longer modifying the original test.
The point of the new one is to avoid skipping any blocks and to validate strict monotonicity after reaching 0 — i.e., a more thorough validation than before.
(https://github.com/bitcoin/bitcoin/pull/30699#issuecomment-2338961152)
@fjahr, @brunoerg, please reevaluate the concept NACKs. I'm no longer modifying the original test.
The point of the new one is to avoid skipping any blocks and to validate strict monotonicity after reaching 0 — i.e., a more thorough validation than before.
💬 fjahr commented on pull request "test: add subsidy sum test, iterating every block":
(https://github.com/bitcoin/bitcoin/pull/30699#issuecomment-2338991268)
> I'm no longer modifying the original test.
You are still modifying the original test and even if you would remove those modifications I still think this is not worth the effort of review. Your new test doesn't add anything of substantial value and I would not be surprised if in a few weeks someone opens a PR to remove the first test because there is already another test that does the same thing which leads to the same result as a rewrite. Then we will have to have another argument about the
...
(https://github.com/bitcoin/bitcoin/pull/30699#issuecomment-2338991268)
> I'm no longer modifying the original test.
You are still modifying the original test and even if you would remove those modifications I still think this is not worth the effort of review. Your new test doesn't add anything of substantial value and I would not be surprised if in a few weeks someone opens a PR to remove the first test because there is already another test that does the same thing which leads to the same result as a rewrite. Then we will have to have another argument about the
...
💬 l0rinc commented on pull request "test: add subsidy sum test, iterating every block":
(https://github.com/bitcoin/bitcoin/pull/30699#issuecomment-2339016088)
> You are still modifying the original test
No, it does the same steps as before.
> I still think this is not worth the effort of review
Your NACK will prevent others from exerting that effort, so what's the real reason for treating this area differently than the rest of the codebase?
I've explained my reasoning for why I think we need to be more thorough, hence my PR.
(https://github.com/bitcoin/bitcoin/pull/30699#issuecomment-2339016088)
> You are still modifying the original test
No, it does the same steps as before.
> I still think this is not worth the effort of review
Your NACK will prevent others from exerting that effort, so what's the real reason for treating this area differently than the rest of the codebase?
I've explained my reasoning for why I think we need to be more thorough, hence my PR.
✅ l0rinc closed a pull request: "refactor: Migrate EmplaceCoinInternalDANGER to try_emplace"
(https://github.com/bitcoin/bitcoin/pull/30637)
(https://github.com/bitcoin/bitcoin/pull/30637)
🤔 hodlinator reviewed a pull request: "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors"
(https://github.com/bitcoin/bitcoin/pull/30765#pullrequestreview-2290825978)
Concept ACK 4574e45a99d8e11ccadd839f2ef8a80356e8955d.
Sad to see my weird fix in https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1748936794 go /s, but also glad the workaround was so smooth. Hope that case won't show up again or one of us remembers it.
(https://github.com/bitcoin/bitcoin/pull/30765#pullrequestreview-2290825978)
Concept ACK 4574e45a99d8e11ccadd839f2ef8a80356e8955d.
Sad to see my weird fix in https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1748936794 go /s, but also glad the workaround was so smooth. Hope that case won't show up again or one of us remembers it.
💬 hodlinator commented on pull request "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750849674)
IMO `Append` -> `Push` to stay more consistent with the OP-codes themselves (if we keep this change of breaking out named methods from `operator<<`).
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750849674)
IMO `Append` -> `Push` to stay more consistent with the OP-codes themselves (if we keep this change of breaking out named methods from `operator<<`).