🤔 l0rinc reviewed a pull request: "refactor: Use std::span over Span"
(https://github.com/bitcoin/bitcoin/pull/31519#pullrequestreview-2594178508)
Docker seems to complain with:
> You have reached your pull rate limit
But locally all tests were passing for me.
I think this is an important change, I'd just a like a few minor changes (like remaining "Spans" references), and I'd prefer doing the unrelated header changes in a different PR for transparency.
(https://github.com/bitcoin/bitcoin/pull/31519#pullrequestreview-2594178508)
Docker seems to complain with:
> You have reached your pull rate limit
But locally all tests were passing for me.
I think this is an important change, I'd just a like a few minor changes (like remaining "Spans" references), and I'd prefer doing the unrelated header changes in a different PR for transparency.
💬 l0rinc commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941963898)
shouldn't these go to the [developer notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md?plain=1#L859-L863) now? We don't have files for other std types locally which explain their behavior.
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941963898)
shouldn't these go to the [developer notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md?plain=1#L859-L863) now? We don't have files for other std types locally which explain their behavior.
💬 l0rinc commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941965691)
While I find it a good idea to finally get rid of these, I think it may be cleaner to do these in a separate PR, this one's quite heavy already
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941965691)
While I find it a good idea to finally get rid of these, I think it may be cleaner to do these in a separate PR, this one's quite heavy already
💬 maflcko commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941982026)
I can see it both ways. The `span.h` will remain to hold `std::span` helpers, so it seems fine to keep it there. It is also fine to move it the dev notes, but this should be a separate commit, to make review easier. Finally, it is also fine to remove it, as the std lib has documentation and doesn't need additional ones in this repo?
I don't really mind either way and I think anything is fine, so I just try to make it as easy to review for reviewers.
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941982026)
I can see it both ways. The `span.h` will remain to hold `std::span` helpers, so it seems fine to keep it there. It is also fine to move it the dev notes, but this should be a separate commit, to make review easier. Finally, it is also fine to remove it, as the std lib has documentation and doesn't need additional ones in this repo?
I don't really mind either way and I think anything is fine, so I just try to make it as easy to review for reviewers.
💬 l0rinc commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941982796)
Maybe something like:
```suggestion
// don't work. This makes it possible to use the std::span constructor in a SFINAE
// contexts like in the Spannable function above to detect whether types are or
// aren't compatible with std::span at compile time.
```
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941982796)
Maybe something like:
```suggestion
// don't work. This makes it possible to use the std::span constructor in a SFINAE
// contexts like in the Spannable function above to detect whether types are or
// aren't compatible with std::span at compile time.
```
💬 maflcko commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941983133)
I think it is fine. It is the short version of "The span objects must be 32 bytes each."
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941983133)
I think it is fine. It is the short version of "The span objects must be 32 bytes each."
💬 l0rinc commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941986868)
I find the suggestions helpful, I'm fine with moving them to doc separately.
The `span's` should likely be adjusted here as well, maybe to `instances of std::span` or something similar.
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941986868)
I find the suggestions helpful, I'm fine with moving them to doc separately.
The `span's` should likely be adjusted here as well, maybe to `instances of std::span` or something similar.
💬 l0rinc commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941987358)
Thanks, please resolve it
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941987358)
Thanks, please resolve it
💬 theuni commented on pull request "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows":
(https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2635223309)
> Rebased. The commit messages have been amended.
>
> @theuni
>
> > Wait, I take that back. It seems like we could still get 2 separate `cs_main`s here: one in the lib and the other in the binary. ACK retracted while I investigate.
>
> That's an interesting observation. Could you elaborate on how you think the two separate `cs_main` instances might arise?
>
> If there's a relevant section in the documentation or standard that you believe applies, pointing it out would be helpful for
...
(https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2635223309)
> Rebased. The commit messages have been amended.
>
> @theuni
>
> > Wait, I take that back. It seems like we could still get 2 separate `cs_main`s here: one in the lib and the other in the binary. ACK retracted while I investigate.
>
> That's an interesting observation. Could you elaborate on how you think the two separate `cs_main` instances might arise?
>
> If there's a relevant section in the documentation or standard that you believe applies, pointing it out would be helpful for
...
💬 theuni commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941993667)
This looks like a good use-case for a static extent :)
(as a follow-up)
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941993667)
This looks like a good use-case for a static extent :)
(as a follow-up)
💬 maflcko commented on issue "bitcoind debug log file is growing without limit":
(https://github.com/bitcoin/bitcoin/issues/31795#issuecomment-2635297607)
https://bitcoin.stackexchange.com/questions/18857/how-to-limit-the-debug-log-file-while-running-bitcoind
Usually the issue tracker is used to track technical issues related to the Bitcoin Core code base.
General bitcoin questions and/or support requests are best directed to the [Bitcoin StackExchange](https://bitcoin.stackexchange.com) or the `#bitcoin` IRC channel on Libera Chat, or one of the Bitcoin subreddits, or any other place that you feel is well suited.
(https://github.com/bitcoin/bitcoin/issues/31795#issuecomment-2635297607)
https://bitcoin.stackexchange.com/questions/18857/how-to-limit-the-debug-log-file-while-running-bitcoind
Usually the issue tracker is used to track technical issues related to the Bitcoin Core code base.
General bitcoin questions and/or support requests are best directed to the [Bitcoin StackExchange](https://bitcoin.stackexchange.com) or the `#bitcoin` IRC channel on Libera Chat, or one of the Bitcoin subreddits, or any other place that you feel is well suited.
🤔 fjahr reviewed a pull request: "mining: bugfix: Fix duplicate coinbase tx weight reservation"
(https://github.com/bitcoin/bitcoin/pull/31384#pullrequestreview-2594288643)
I left comments there but I don't want to bikeshed the release notes too much, so feel free to ignore them unless you have to retouch or don't want to address them.
(https://github.com/bitcoin/bitcoin/pull/31384#pullrequestreview-2594288643)
I left comments there but I don't want to bikeshed the release notes too much, so feel free to ignore them unless you have to retouch or don't want to address them.
💬 fjahr commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1942035850)
nit
```suggestion
Before this pull request, the policy default for the maximum block weight was `3,996,000 WU`, calculated by
```
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1942035850)
nit
```suggestion
Before this pull request, the policy default for the maximum block weight was `3,996,000 WU`, calculated by
```
💬 fjahr commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1942039833)
Shouldn't this now be this?
```suggestion
const auto block_adjusted_max_weight = MAX_BLOCK_WEIGHT - MINIMUM_BLOCK_RESERVED_WEIGHT;
```
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1942039833)
Shouldn't this now be this?
```suggestion
const auto block_adjusted_max_weight = MAX_BLOCK_WEIGHT - MINIMUM_BLOCK_RESERVED_WEIGHT;
```
💬 fjahr commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1942047954)
nit: I find it a bit confusing to mention policy here. While these values are stored in the policy file, this reads a bit like blocks wouldn't propagate when exceeding the values set there. But maybe that's just me...
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1942047954)
nit: I find it a bit confusing to mention policy here. While these values are stored in the policy file, this reads a bit like blocks wouldn't propagate when exceeding the values set there. But maybe that's just me...
💬 fjahr commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1942057070)
This is repeating what is already explained above.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1942057070)
This is repeating what is already explained above.
💬 ryanofsky commented on issue "test: 32-bit Clang `ipc_test` failure at `-O0`":
(https://github.com/bitcoin/bitcoin/issues/31772#issuecomment-2635384042)
I tried to repeat previous steps rebuilding depends and test_bitcoin in container to figure out what I was doing that caused the test to stop crashing, but it seems to crash reliably, so I can't work out what I other changes I might have made while trying -fabi-version flags in https://github.com/bitcoin/bitcoin/issues/31772#issuecomment-2634917217 that would have caused it not to crash.
Additionally, I went back to original build and debugged it with GDB, and could easily step through and see
...
(https://github.com/bitcoin/bitcoin/issues/31772#issuecomment-2635384042)
I tried to repeat previous steps rebuilding depends and test_bitcoin in container to figure out what I was doing that caused the test to stop crashing, but it seems to crash reliably, so I can't work out what I other changes I might have made while trying -fabi-version flags in https://github.com/bitcoin/bitcoin/issues/31772#issuecomment-2634917217 that would have caused it not to crash.
Additionally, I went back to original build and debugged it with GDB, and could easily step through and see
...
✅ pinheadmz closed an issue: "bitcoind debug log file is growing without limit"
(https://github.com/bitcoin/bitcoin/issues/31795)
(https://github.com/bitcoin/bitcoin/issues/31795)
⚠️ lilin998 opened an issue: "signet /README.md"
(https://github.com/bitcoin/bitcoin/issues/31798)
MINER="./contrib/signet/miner"
GRIND="./build/src/bitcoin-util grind"
$MINER calibrate --grind-cmd="$GRIND"
nbits=1e00f403 for 25s average mining time
CLI="./build/src/bitcoin-cli -conf=mysignet.conf"
ADDR=$($CLI -signet getnewaddress)
NBITS=1e00f403
$MINER --cli="$CLI" generate --grind-cmd="$GRIND" --address="$ADDR" --nbits=$NBITS
PSBT signing failed
How to solve this problem PSBT signing failed
(https://github.com/bitcoin/bitcoin/issues/31798)
MINER="./contrib/signet/miner"
GRIND="./build/src/bitcoin-util grind"
$MINER calibrate --grind-cmd="$GRIND"
nbits=1e00f403 for 25s average mining time
CLI="./build/src/bitcoin-cli -conf=mysignet.conf"
ADDR=$($CLI -signet getnewaddress)
NBITS=1e00f403
$MINER --cli="$CLI" generate --grind-cmd="$GRIND" --address="$ADDR" --nbits=$NBITS
PSBT signing failed
How to solve this problem PSBT signing failed
💬 pinheadmz commented on issue "signet /README.md":
(https://github.com/bitcoin/bitcoin/issues/31798#issuecomment-2635518680)
> PSBT signing failed
This happens when the wallet does not have the private key to sign valid signet blocks that satisfy the challenge
(https://github.com/bitcoin/bitcoin/issues/31798#issuecomment-2635518680)
> PSBT signing failed
This happens when the wallet does not have the private key to sign valid signet blocks that satisfy the challenge