💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2124894458)
ignoring because the constant gets deleted
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2124894458)
ignoring because the constant gets deleted
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2124891663)
added a test that has 10 evictions in 1 go
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2124891663)
added a test that has 10 evictions in 1 go
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2124907696)
Restructured this way, with the help of 🪄 ✨AI ✨
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2124907696)
Restructured this way, with the help of 🪄 ✨AI ✨
💬 davidgumberg commented on pull request "test: apply microsecond precision to test framework logging":
(https://github.com/bitcoin/bitcoin/pull/32676#issuecomment-2937200960)
Tested ACK https://github.com/bitcoin/bitcoin/pull/32676/commits/ed179e0a6528c39b3bca76365f256716f917e19b
Added an `assert(0)` to `feature_abortnode.py` and on this branch, microsecond times are printed, on master (5471e29d057049d2b5a) they are not.
Didn't reproduce the described situation in the PR, but it makes sense that this would result in nonsensical ordering of logs any time you have sub-ms events, and I can recall having run into this before.
<details>
<summary>
### Test
...
(https://github.com/bitcoin/bitcoin/pull/32676#issuecomment-2937200960)
Tested ACK https://github.com/bitcoin/bitcoin/pull/32676/commits/ed179e0a6528c39b3bca76365f256716f917e19b
Added an `assert(0)` to `feature_abortnode.py` and on this branch, microsecond times are printed, on master (5471e29d057049d2b5a) they are not.
Didn't reproduce the described situation in the PR, but it makes sense that this would result in nonsensical ordering of logs any time you have sub-ms events, and I can recall having run into this before.
<details>
<summary>
### Test
...
💬 enirox001 commented on issue "Potential data race on fHavePruned flag":
(https://github.com/bitcoin/bitcoin/issues/21627#issuecomment-2937210740)
Sure, I can give it a go
(https://github.com/bitcoin/bitcoin/issues/21627#issuecomment-2937210740)
Sure, I can give it a go
💬 davidgumberg commented on pull request "deps: Bump lief to 0.16.6":
(https://github.com/bitcoin/bitcoin/pull/32431#issuecomment-2937216785)
```console
$ find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
```
```
9607cec72e511df72d1d07ef850015172d6c2707604f46694c019cd37903c65b guix-build-74dfa4155037/output/aarch64-linux-gnu/SHA256SUMS.part
30916e57efaa2bfd4265d840cde7049c863c4d35cec9d5e70681e7bc428e2d14 guix-build-74dfa4155037/output/aarch64-linux-gnu/bitcoin-74dfa4155037-aarch64-linux-gnu-debug.tar.gz
c462783e946c5e3e61a6a4ab4be1b8ddb6db9be07194d3241b43a8c
...
(https://github.com/bitcoin/bitcoin/pull/32431#issuecomment-2937216785)
```console
$ find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
```
```
9607cec72e511df72d1d07ef850015172d6c2707604f46694c019cd37903c65b guix-build-74dfa4155037/output/aarch64-linux-gnu/SHA256SUMS.part
30916e57efaa2bfd4265d840cde7049c863c4d35cec9d5e70681e7bc428e2d14 guix-build-74dfa4155037/output/aarch64-linux-gnu/bitcoin-74dfa4155037-aarch64-linux-gnu-debug.tar.gz
c462783e946c5e3e61a6a4ab4be1b8ddb6db9be07194d3241b43a8c
...
💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2124941224)
Done
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2124941224)
Done
💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2124941299)
Done
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2124941299)
Done
💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2124941401)
Done
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2124941401)
Done
💬 achow101 commented on pull request "rpc: Use type-safe HelpException":
(https://github.com/bitcoin/bitcoin/pull/32660#issuecomment-2937298230)
> Are you sure? What are the exact steps to reproduce?
Nope.
Not sure what I did wrong yesterday, but tried again and it does seem to work.
(https://github.com/bitcoin/bitcoin/pull/32660#issuecomment-2937298230)
> Are you sure? What are the exact steps to reproduce?
Nope.
Not sure what I did wrong yesterday, but tried again and it does seem to work.
💬 achow101 commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2124994068)
This test with `now` is basically the same as `never`. Since the distinction is that `now` does actually rescan a little bit, it would be good to add in the setup for these tests a transaction that would be scanned by `now` but not `never`.
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2124994068)
This test with `now` is basically the same as `never`. Since the distinction is that `now` does actually rescan a little bit, it would be good to add in the setup for these tests a transaction that would be scanned by `now` but not `never`.
🤔 ismaelsadeeq reviewed a pull request: "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store"
(https://github.com/bitcoin/bitcoin/pull/32427#pullrequestreview-2894302384)
> Moving away from leveldb opens the door towards doing this in parallel.
For this reason, I am Concept ACK.
No opinion on the approach yet; I am still studying the PR and prev discussion.
It might be a little too early for this but I'm excited and tried testing it out on Signet.
Just by building the branch and running the node on Signet, it crashes. See logs (I didnt start the node with `-reindex` option or maybe I might be doing something wrong though):
https://gist.github.com/is
...
(https://github.com/bitcoin/bitcoin/pull/32427#pullrequestreview-2894302384)
> Moving away from leveldb opens the door towards doing this in parallel.
For this reason, I am Concept ACK.
No opinion on the approach yet; I am still studying the PR and prev discussion.
It might be a little too early for this but I'm excited and tried testing it out on Signet.
Just by building the branch and running the node on Signet, it crashes. See logs (I didnt start the node with `-reindex` option or maybe I might be doing something wrong though):
https://gist.github.com/is
...
🤔 fjahr reviewed a pull request: "tests: Expand HTTP coverage to assert libevent behavior"
(https://github.com/bitcoin/bitcoin/pull/32408#pullrequestreview-2894320133)
utACK f16c8c67bf137e64fbeea1242431baaa915a5c53
Looks good to me, I would consider my comments non-blocking and could potentially be addressed in a follow-up.
(https://github.com/bitcoin/bitcoin/pull/32408#pullrequestreview-2894320133)
utACK f16c8c67bf137e64fbeea1242431baaa915a5c53
Looks good to me, I would consider my comments non-blocking and could potentially be addressed in a follow-up.
💬 fjahr commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2125071947)
nit: I haven't looked at the actual data but this seems a bit fragile, if the second "result" is at the very end of res, the test would continue here but then fail below. Not sure how real that threat is but it could also be easily mitigated by doing one more `recv` I guess.
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2125071947)
nit: I haven't looked at the actual data but this seems a bit fragile, if the second "result" is at the very end of res, the test would continue here but then fail below. Not sure how real that threat is but it could also be easily mitigated by doing one more `recv` I guess.
💬 fjahr commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2125070216)
nit: Could use a helper function to get rid of some duplication here (and even beyond the changes here across the whole test file).
```
def _get_http_connection(self, url_node):
conn = http.client.HTTPConnection(url_node.hostname, url_node.port)
conn.connect()
sock = conn.sock
return conn, sock
```
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2125070216)
nit: Could use a helper function to get rid of some duplication here (and even beyond the changes here across the whole test file).
```
def _get_http_connection(self, url_node):
conn = http.client.HTTPConnection(url_node.hostname, url_node.port)
conn.connect()
sock = conn.sock
return conn, sock
```
💬 tnndbtc commented on issue "bitcoind shouldn't fail to progress with synchronization: endless [leveldb] Generated table ... logs":
(https://github.com/bitcoin/bitcoin/issues/31882#issuecomment-2937789647)
@GregTonoski Just curious, how do you get the pruned node of 550MB? I'd like to explore a little more about this issue but searched on the internet and not able to find a easy solution to get a pruned node to start with.
(https://github.com/bitcoin/bitcoin/issues/31882#issuecomment-2937789647)
@GregTonoski Just curious, how do you get the pruned node of 550MB? I'd like to explore a little more about this issue but searched on the internet and not able to find a easy solution to get a pruned node to start with.
👋 portlandhodl's pull request is ready for review: "Enhanced error messages for invalid network prefix during address parsing."
(https://github.com/bitcoin/bitcoin/pull/27260)
(https://github.com/bitcoin/bitcoin/pull/27260)
💬 portlandhodl commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-2938182776)
https://github.com/bitcoin/bitcoin/pull/27260/commits/2f2470e844c79695df0140103c73d742371554cc
This is the one! with this commit I have enabled multi network functional testing [Signet, Mainnet] this enables proof that under different networks the results will match expectations. This also had a few benefits too!
Previously commented out tests are now valid!
A lot of errors are now properly handed on a per network basis!
Thanks, and ready for review!
(https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-2938182776)
https://github.com/bitcoin/bitcoin/pull/27260/commits/2f2470e844c79695df0140103c73d742371554cc
This is the one! with this commit I have enabled multi network functional testing [Signet, Mainnet] this enables proof that under different networks the results will match expectations. This also had a few benefits too!
Previously commented out tests are now valid!
A lot of errors are now properly handed on a per network basis!
Thanks, and ready for review!
🤔 ryanofsky reviewed a pull request: "ipc: add bitcoin-mine test program"
(https://github.com/bitcoin/bitcoin/pull/30437#pullrequestreview-2894793634)
Thanks for the reviews!
Rebased 6645095e6ae0d9c208a47fc7322d859c4fe6afb0 -> 27c6576aba5c59402b8844703e04face2f41578c ([`pr/mine.23`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.23) -> [`pr/mine.24`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.24), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.23-rebase..pr/mine.24)) due to conflict with #31375, also bringing over some changes from #32297 and implementing some suggestions.
re: https://github.com/bitcoin/bi
...
(https://github.com/bitcoin/bitcoin/pull/30437#pullrequestreview-2894793634)
Thanks for the reviews!
Rebased 6645095e6ae0d9c208a47fc7322d859c4fe6afb0 -> 27c6576aba5c59402b8844703e04face2f41578c ([`pr/mine.23`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.23) -> [`pr/mine.24`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.24), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.23-rebase..pr/mine.24)) due to conflict with #31375, also bringing over some changes from #32297 and implementing some suggestions.
re: https://github.com/bitcoin/bi
...
💬 ryanofsky commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2125412533)
re: https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2112219834
> I attempted it in https://github.com/ismaelsadeeq/bitcoin/commit/b90c02ea22432c465c947937ba7c655db5d29bbc
Thanks, this seems like a possible direction to go in the future. But I could also see it going in a different direction of calling more mining functions and providing more complete functional test coverage of the mining interface.
I think the other direction might be more practically useful so don't want to
...
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2125412533)
re: https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2112219834
> I attempted it in https://github.com/ismaelsadeeq/bitcoin/commit/b90c02ea22432c465c947937ba7c655db5d29bbc
Thanks, this seems like a possible direction to go in the future. But I could also see it going in a different direction of calling more mining functions and providing more complete functional test coverage of the mining interface.
I think the other direction might be more practically useful so don't want to
...