🚀 fanquake merged a pull request: "depends: consolidate dependency docs"
(https://github.com/bitcoin/bitcoin/pull/30204)
(https://github.com/bitcoin/bitcoin/pull/30204)
📝 Sjors opened a pull request: "net: have GetReceivedMessage() return bytes"
(https://github.com/bitcoin/bitcoin/pull/30213)
Fixes #30209.
Previously `GetReceivedMessage()` returned a CNetMessage. Have the caller process the bytes instead. Similarly `SetMessageToSend` needed a `CSerializedNetMsg`, give it bytes instead.
This makes the `Transport` class usable for future message types that don't inherit from CNetMessage.
TODO:
- [ ] clean up
- [ ] fix tests
- [ ] implement `SetMessageToSend`
(https://github.com/bitcoin/bitcoin/pull/30213)
Fixes #30209.
Previously `GetReceivedMessage()` returned a CNetMessage. Have the caller process the bytes instead. Similarly `SetMessageToSend` needed a `CSerializedNetMsg`, give it bytes instead.
This makes the `Transport` class usable for future message types that don't inherit from CNetMessage.
TODO:
- [ ] clean up
- [ ] fix tests
- [ ] implement `SetMessageToSend`
💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1622525854)
@theuni Nice idea, done.
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1622525854)
@theuni Nice idea, done.
💬 Sjors commented on issue "Make Transport independent of CNetMessage and CSerializedNetMsg":
(https://github.com/bitcoin/bitcoin/issues/30209#issuecomment-2142404400)
Easier said than done... very rough draft in #30213
(https://github.com/bitcoin/bitcoin/issues/30209#issuecomment-2142404400)
Easier said than done... very rough draft in #30213
💬 Sjors commented on pull request "net: have GetReceivedMessage() return bytes":
(https://github.com/bitcoin/bitcoin/pull/30213#issuecomment-2142415999)
v2 is fairly straight-forward, but v1 is not.
(https://github.com/bitcoin/bitcoin/pull/30213#issuecomment-2142415999)
v2 is fairly straight-forward, but v1 is not.
💬 sipa commented on pull request "net: have GetReceivedMessage() return bytes":
(https://github.com/bitcoin/bitcoin/pull/30213#discussion_r1622531541)
Please don't do this; the whole point of having separate transport classes is hiding the complexity of dealing with the difference from the rest of the network code.
(https://github.com/bitcoin/bitcoin/pull/30213#discussion_r1622531541)
Please don't do this; the whole point of having separate transport classes is hiding the complexity of dealing with the difference from the rest of the network code.
💬 sipa commented on pull request "net: have GetReceivedMessage() return bytes":
(https://github.com/bitcoin/bitcoin/pull/30213#issuecomment-2142430707)
Would an alternative approach be that `V1Transport` and `V2Transport` always just treat the first 12 bytes of a message as the command type (as in on decode they'd produce a message with "length" 12+payload_len, etc). That would mean the `Transport` interface works purely on bytes, and could be usable for protocols that don't deal with things that have a command type.
Another option is having two layers: a "raw transport" interface that works on bytes, and a layer "command-based transport" in
...
(https://github.com/bitcoin/bitcoin/pull/30213#issuecomment-2142430707)
Would an alternative approach be that `V1Transport` and `V2Transport` always just treat the first 12 bytes of a message as the command type (as in on decode they'd produce a message with "length" 12+payload_len, etc). That would mean the `Transport` interface works purely on bytes, and could be usable for protocols that don't deal with things that have a command type.
Another option is having two layers: a "raw transport" interface that works on bytes, and a layer "command-based transport" in
...
📝 ryanofsky opened a pull request: "refactor: Improve assumeutxo state representation"
(https://github.com/bitcoin/bitcoin/pull/30214)
This PR contains the first part of #28608, which tries to make assumeutxo code more maintainable, and improve it by not locking `cs_main` for a long time when the snapshot block is connected, and by deleting the snapshot validation chainstate when it is no longer used, instead of waiting until the next restart.
The changes in this PR are just refactoring. They make `Chainstate` objects self-contained, so for example, it is possible to determine what blocks to connect to a chainstate without q
...
(https://github.com/bitcoin/bitcoin/pull/30214)
This PR contains the first part of #28608, which tries to make assumeutxo code more maintainable, and improve it by not locking `cs_main` for a long time when the snapshot block is connected, and by deleting the snapshot validation chainstate when it is no longer used, instead of waiting until the next restart.
The changes in this PR are just refactoring. They make `Chainstate` objects self-contained, so for example, it is possible to determine what blocks to connect to a chainstate without q
...
💬 setavenger commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1622550401)
Cool! Would you mind sharing the current disk usage here https://github.com/setavenger/blindbit-oracle/issues/15
it's been a while since I've done a full mainnet sync.
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1622550401)
Cool! Would you mind sharing the current disk usage here https://github.com/setavenger/blindbit-oracle/issues/15
it's been a while since I've done a full mainnet sync.
🤔 marcofleon reviewed a pull request: "fuzz: add more coverage for `ScriptPubKeyMan`"
(https://github.com/bitcoin/bitcoin/pull/30134#pullrequestreview-2091071703)
Tested ACK e3249f21111f1dd4beb66f10af933c34a36c30ac. I ran the updated harness for ~9 hours on an empty corpus, generated a coverage report, and checked that the new functions mentioned were hit. Coverage of `scriptpubkeyman.cpp` increased.
(https://github.com/bitcoin/bitcoin/pull/30134#pullrequestreview-2091071703)
Tested ACK e3249f21111f1dd4beb66f10af933c34a36c30ac. I ran the updated harness for ~9 hours on an empty corpus, generated a coverage report, and checked that the new functions mentioned were hit. Coverage of `scriptpubkeyman.cpp` increased.
🤔 mzumsande reviewed a pull request: "net: log connections failures via SOCKS5 with less severity"
(https://github.com/bitcoin/bitcoin/pull/30064#pullrequestreview-2091095163)
Code Review ACK f3cfbd65f54b5b6423d786fb8850cece05af603e
(https://github.com/bitcoin/bitcoin/pull/30064#pullrequestreview-2091095163)
Code Review ACK f3cfbd65f54b5b6423d786fb8850cece05af603e
💬 Sjors commented on pull request "net: have GetReceivedMessage() return bytes":
(https://github.com/bitcoin/bitcoin/pull/30213#issuecomment-2142483329)
In `Sv2Transport` (see [here](https://github.com/bitcoin/bitcoin/pull/29432/files#diff-a84ee692989f2fa3ed26266d59b1d22b608a4f2cfb0ef1faa910a5b96c8ebeef) ) the encrypted headers contains both the message length and size. In `V2Transport` we first get the size and then get the type and its content. And in `V1Transport` we first get the message type and then the size followed by the content.
At the very minimum `Transport` needs to know the message size. Currently the `V1Transport` and `V2Transp
...
(https://github.com/bitcoin/bitcoin/pull/30213#issuecomment-2142483329)
In `Sv2Transport` (see [here](https://github.com/bitcoin/bitcoin/pull/29432/files#diff-a84ee692989f2fa3ed26266d59b1d22b608a4f2cfb0ef1faa910a5b96c8ebeef) ) the encrypted headers contains both the message length and size. In `V2Transport` we first get the size and then get the type and its content. And in `V1Transport` we first get the message type and then the size followed by the content.
At the very minimum `Transport` needs to know the message size. Currently the `V1Transport` and `V2Transp
...
💬 hebasto commented on pull request "depends: remove `FORCE_USE_SYSTEM_CLANG`":
(https://github.com/bitcoin/bitcoin/pull/30201#issuecomment-2142491843)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/30201#issuecomment-2142491843)
Concept ACK.
✅ fanquake closed a pull request: "refactor: refactored platform assignment into get_platform function"
(https://github.com/bitcoin/bitcoin/pull/29971)
(https://github.com/bitcoin/bitcoin/pull/29971)
💬 fanquake commented on pull request "refactor: refactored platform assignment into get_platform function":
(https://github.com/bitcoin/bitcoin/pull/29971#issuecomment-2142510334)
Thanks for the contribution, however we are going to leave this code as-is for now.
(https://github.com/bitcoin/bitcoin/pull/29971#issuecomment-2142510334)
Thanks for the contribution, however we are going to leave this code as-is for now.
💬 fanquake commented on pull request "RFC: depends: add release type to CMake builds":
(https://github.com/bitcoin/bitcoin/pull/29962#issuecomment-2142512483)
This "seems" like the right thing to do, but I'm also not sure. Could you rebase here, to incorporate other recent CMake changes, and so it's easier to test?
(https://github.com/bitcoin/bitcoin/pull/29962#issuecomment-2142512483)
This "seems" like the right thing to do, but I'm also not sure. Could you rebase here, to incorporate other recent CMake changes, and so it's easier to test?
💬 fanquake commented on pull request "Lint: support running individual rust linters and improve subtree exclusion":
(https://github.com/bitcoin/bitcoin/pull/29965#issuecomment-2142513388)
> Looks like the conflicting https://github.com/bitcoin/bitcoin/pull/30034 is close(r) to merge, if you want to review it
This is now merged. Can you rebase here?
(https://github.com/bitcoin/bitcoin/pull/29965#issuecomment-2142513388)
> Looks like the conflicting https://github.com/bitcoin/bitcoin/pull/30034 is close(r) to merge, if you want to review it
This is now merged. Can you rebase here?
💬 fanquake commented on pull request "depends: Use `CC_FOR_BUILD` for `config.guess `":
(https://github.com/bitcoin/bitcoin/pull/29963#issuecomment-2142519151)
Is this still an issue given recent CMake changes?
(https://github.com/bitcoin/bitcoin/pull/29963#issuecomment-2142519151)
Is this still an issue given recent CMake changes?
👍 hebasto approved a pull request: "build: remove `--enable-lcov-branch-coverage`"
(https://github.com/bitcoin/bitcoin/pull/30192#pullrequestreview-2091159773)
ACK cbd4640ede92a1a5d7b7c1367eb7c00a9f476c62, tested on Ubuntu 22.04.
(https://github.com/bitcoin/bitcoin/pull/30192#pullrequestreview-2091159773)
ACK cbd4640ede92a1a5d7b7c1367eb7c00a9f476c62, tested on Ubuntu 22.04.
💬 hebasto commented on pull request "build: remove `--enable-lcov-branch-coverage`":
(https://github.com/bitcoin/bitcoin/pull/30192#discussion_r1622610069)
Not only "when configuring". Providing `LCOV_OPTS` variable when building the `cov` or `cov_fuzz` targets also works and improves flexibility:
```
./configure --enable-lcov CXXFLAGS="-fprofile-update=prefer-atomic"
make
make cov LCOV_OPTS="--rc branch_coverage=1 --ignore-errors mismatch"
```
(https://github.com/bitcoin/bitcoin/pull/30192#discussion_r1622610069)
Not only "when configuring". Providing `LCOV_OPTS` variable when building the `cov` or `cov_fuzz` targets also works and improves flexibility:
```
./configure --enable-lcov CXXFLAGS="-fprofile-update=prefer-atomic"
make
make cov LCOV_OPTS="--rc branch_coverage=1 --ignore-errors mismatch"
```