💬 vasild commented on pull request "fuzz: Make FuzzedSock fuzz friendlier":
(https://github.com/bitcoin/bitcoin/pull/30211#discussion_r1623328097)
I guess using `ConsumeRandomLengthByteVector()` is slower than `ConsumeBytes()` because it uses an intermediate `std::string`, but is probably insignificant.
The comment says:
```cpp
// Returns a std::string of length from 0 to |max_length|. When it runs out of
// input data, returns what remains of the input. Designed to be more stable
// with respect to a fuzzer inserting characters than just picking a random
// length and then consuming that many bytes with |ConsumeBytes|.
inline s
...
(https://github.com/bitcoin/bitcoin/pull/30211#discussion_r1623328097)
I guess using `ConsumeRandomLengthByteVector()` is slower than `ConsumeBytes()` because it uses an intermediate `std::string`, but is probably insignificant.
The comment says:
```cpp
// Returns a std::string of length from 0 to |max_length|. When it runs out of
// input data, returns what remains of the input. Designed to be more stable
// with respect to a fuzzer inserting characters than just picking a random
// length and then consuming that many bytes with |ConsumeBytes|.
inline s
...
💬 vasild commented on pull request "fuzz: Make FuzzedSock fuzz friendlier":
(https://github.com/bitcoin/bitcoin/pull/30211#discussion_r1623330824)
Wow! Good catch! Maybe this deserves a comment because there is deeper reason for the way it is written than one may see at a first glance. (aka to prevent somebody restoring it back to `? requested : 0` in a future refactor, code reorganization).
(https://github.com/bitcoin/bitcoin/pull/30211#discussion_r1623330824)
Wow! Good catch! Maybe this deserves a comment because there is deeper reason for the way it is written than one may see at a first glance. (aka to prevent somebody restoring it back to `? requested : 0` in a future refactor, code reorganization).
🤔 glozow reviewed a pull request: "fuzz: increase `txorphan` harness stability"
(https://github.com/bitcoin/bitcoin/pull/30186#pullrequestreview-2092227439)
utACK 8defc182a31d828ad0f53ebf7e3be9e9cfc42d09
(https://github.com/bitcoin/bitcoin/pull/30186#pullrequestreview-2092227439)
utACK 8defc182a31d828ad0f53ebf7e3be9e9cfc42d09
💬 glozow commented on pull request "policy: bump TX_MAX_STANDARD_VERSION to 3":
(https://github.com/bitcoin/bitcoin/pull/29496#discussion_r1623341181)
thanks! fixed these now
(https://github.com/bitcoin/bitcoin/pull/29496#discussion_r1623341181)
thanks! fixed these now
💬 TheCharlatan commented on pull request "RFC: Remove boost usage from kernel api / headers":
(https://github.com/bitcoin/bitcoin/pull/28335#issuecomment-2143743732)
Closing this. There is some work being done towards removing boost from the mempool wholesale. Since that would be preferable, pruning just the headers can be revisited in case that effort proves unsuccessful.
(https://github.com/bitcoin/bitcoin/pull/28335#issuecomment-2143743732)
Closing this. There is some work being done towards removing boost from the mempool wholesale. Since that would be preferable, pruning just the headers can be revisited in case that effort proves unsuccessful.
✅ TheCharlatan closed a pull request: "RFC: Remove boost usage from kernel api / headers"
(https://github.com/bitcoin/bitcoin/pull/28335)
(https://github.com/bitcoin/bitcoin/pull/28335)
💬 hebasto commented on issue "build, android: `make apk` fails if depends source cache is in a non-default location":
(https://github.com/bitcoin/bitcoin/issues/22522#issuecomment-2143768165)
This can be closed as builds for Android are [not possible](https://github.com/bitcoin/bitcoin/issues/29360) in the master branch.
(https://github.com/bitcoin/bitcoin/issues/22522#issuecomment-2143768165)
This can be closed as builds for Android are [not possible](https://github.com/bitcoin/bitcoin/issues/29360) in the master branch.
💬 theStack commented on pull request "test: improve BDB parser (handle internal/overflow pages, support all page sizes)":
(https://github.com/bitcoin/bitcoin/pull/30125#discussion_r1623397692)
The main reason for using `struct.unpack` in this PR is to be consistent with the already existing code in the bdb module (i.e. avoiding to mix up different serialization methods), and I didn't want to introduce large refactoring changes in this PR. I strongly agree though that `int.{to,from}_bytes` is preferred and support #29401.
(https://github.com/bitcoin/bitcoin/pull/30125#discussion_r1623397692)
The main reason for using `struct.unpack` in this PR is to be consistent with the already existing code in the bdb module (i.e. avoiding to mix up different serialization methods), and I didn't want to introduce large refactoring changes in this PR. I strongly agree though that `int.{to,from}_bytes` is preferred and support #29401.
💬 theStack commented on pull request "test: Remove struct.pack from almost all places":
(https://github.com/bitcoin/bitcoin/pull/29401#issuecomment-2143824209)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/29401#issuecomment-2143824209)
Concept ACK
💬 theStack commented on pull request "doc, rpc: Release notes and follow-ups for #29612":
(https://github.com/bitcoin/bitcoin/pull/30167#discussion_r1623464945)
nit: not a must, but it could be interesting for users to know what exactly has improved with the new format (i.e. enhanced metadata, enabling more detailed error messages etc.)
(https://github.com/bitcoin/bitcoin/pull/30167#discussion_r1623464945)
nit: not a must, but it could be interesting for users to know what exactly has improved with the new format (i.e. enhanced metadata, enabling more detailed error messages etc.)
👍 theStack approved a pull request: "doc, rpc: Release notes and follow-ups for #29612"
(https://github.com/bitcoin/bitcoin/pull/30167#pullrequestreview-2092374868)
utACK efc1b5be8a4696c0db19ba18316b2d4ed09e10f2
(https://github.com/bitcoin/bitcoin/pull/30167#pullrequestreview-2092374868)
utACK efc1b5be8a4696c0db19ba18316b2d4ed09e10f2
💬 davidgumberg commented on pull request "Lint: support running individual rust linters and improve subtree exclusion":
(https://github.com/bitcoin/bitcoin/pull/29965#issuecomment-2143889113)
Rebased to address merge conflict
(https://github.com/bitcoin/bitcoin/pull/29965#issuecomment-2143889113)
Rebased to address merge conflict
💬 hebasto commented on issue "test: SegFault in `ismine_tests` on SunOS / illumos":
(https://github.com/bitcoin/bitcoin/issues/29908#issuecomment-2143979930)
I've tested the recent master branch @ 457e1846d2bf6ef9d54b9ba1a330ba8bbff13091 on the updated system:
```
$ uname -a
SunOS openindiana 5.11 illumos-82079dec87 i86pc i386 i86pc
```
The issue is no longer reproducible.
Closing.
(https://github.com/bitcoin/bitcoin/issues/29908#issuecomment-2143979930)
I've tested the recent master branch @ 457e1846d2bf6ef9d54b9ba1a330ba8bbff13091 on the updated system:
```
$ uname -a
SunOS openindiana 5.11 illumos-82079dec87 i86pc i386 i86pc
```
The issue is no longer reproducible.
Closing.
✅ hebasto closed an issue: "test: SegFault in `ismine_tests` on SunOS / illumos"
(https://github.com/bitcoin/bitcoin/issues/29908)
(https://github.com/bitcoin/bitcoin/issues/29908)
✅ fanquake closed an issue: "build, android: `make apk` fails if depends source cache is in a non-default location"
(https://github.com/bitcoin/bitcoin/issues/22522)
(https://github.com/bitcoin/bitcoin/issues/22522)
📝 hebasto opened a pull request: "build: Fix building `fuzz` binary on on SunOS / illumos"
(https://github.com/bitcoin/bitcoin/pull/30216)
On master branch @ 457e1846d2bf6ef9d54b9ba1a330ba8bbff13091, building the `fuzz` binary fails:
```
$ ./autogen.sh
$ ./configure
$ gmake -C src test/fuzz/fuzz
< snip >
CXX test/fuzz/fuzz-http_request.o
test/fuzz/http_request.cpp:13:10: fatal error: event2/buffer.h: No such file or directory
13 | #include <event2/buffer.h>
| ^~~~~~~~~~~~~~~~~
compilation terminated.
gmake: *** [Makefile:17138: test/fuzz/fuzz-http_request.o] Error 1
gmake: Leaving directory '/ex
...
(https://github.com/bitcoin/bitcoin/pull/30216)
On master branch @ 457e1846d2bf6ef9d54b9ba1a330ba8bbff13091, building the `fuzz` binary fails:
```
$ ./autogen.sh
$ ./configure
$ gmake -C src test/fuzz/fuzz
< snip >
CXX test/fuzz/fuzz-http_request.o
test/fuzz/http_request.cpp:13:10: fatal error: event2/buffer.h: No such file or directory
13 | #include <event2/buffer.h>
| ^~~~~~~~~~~~~~~~~
compilation terminated.
gmake: *** [Makefile:17138: test/fuzz/fuzz-http_request.o] Error 1
gmake: Leaving directory '/ex
...
💬 laib200 commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#issuecomment-2144007809)
براكة الله فيك 😊
(https://github.com/bitcoin/bitcoin/pull/30079#issuecomment-2144007809)
براكة الله فيك 😊
👍 tdb3 approved a pull request: "test: Validate oversized transactions or without inputs"
(https://github.com/bitcoin/bitcoin/pull/29862#pullrequestreview-2092614645)
re ACK for 0aa7db42564408edb41b0d42103d39ba4c2787dc
Re-ran modifications of the tests to induce failure as described in https://github.com/bitcoin/bitcoin/pull/29862#discussion_r1564345633 and https://github.com/bitcoin/bitcoin/pull/29862/commits/0aa7db42564408edb41b0d42103d39ba4c2787dc#r1578682415.
Unexpected reject reasons were caught (succesfully).
(https://github.com/bitcoin/bitcoin/pull/29862#pullrequestreview-2092614645)
re ACK for 0aa7db42564408edb41b0d42103d39ba4c2787dc
Re-ran modifications of the tests to induce failure as described in https://github.com/bitcoin/bitcoin/pull/29862#discussion_r1564345633 and https://github.com/bitcoin/bitcoin/pull/29862/commits/0aa7db42564408edb41b0d42103d39ba4c2787dc#r1578682415.
Unexpected reject reasons were caught (succesfully).
👍 tdb3 approved a pull request: "netbase: extend CreateSock() to support creating arbitrary sockets"
(https://github.com/bitcoin/bitcoin/pull/30202#pullrequestreview-2092618199)
ACK for 77e34ded543e19ba27cab8d0cfc464af27f04bbf
Thanks. Seems like a reasonable change.
Sanity checks performed:
- Synced the last 1.5 day's worth of blocks on signet. Binding and sync seemed to work fine.
- Ran all unit and functional tests (passed).
Left some minor notes (just thinking out loud, so no need to answer/comment unless you feel it's necessary).
(https://github.com/bitcoin/bitcoin/pull/30202#pullrequestreview-2092618199)
ACK for 77e34ded543e19ba27cab8d0cfc464af27f04bbf
Thanks. Seems like a reasonable change.
Sanity checks performed:
- Synced the last 1.5 day's worth of blocks on signet. Binding and sync seemed to work fine.
- Ran all unit and functional tests (passed).
Left some minor notes (just thinking out loud, so no need to answer/comment unless you feel it's necessary).
💬 tdb3 commented on pull request "netbase: extend CreateSock() to support creating arbitrary sockets":
(https://github.com/bitcoin/bitcoin/pull/30202#discussion_r1623667994)
Seems good to add this sanity check to prevent setting non-pertinent sockopts. Might be worthwhile to mention in the PR description that in addition to extending arguments, some extra safety checks were put in place.
(https://github.com/bitcoin/bitcoin/pull/30202#discussion_r1623667994)
Seems good to add this sanity check to prevent setting non-pertinent sockopts. Might be worthwhile to mention in the PR description that in addition to extending arguments, some extra safety checks were put in place.