👍 hodlinator approved a pull request: "contrib: Make deterministic-coverage error messages more readable"
(https://github.com/bitcoin/bitcoin/pull/32074#pullrequestreview-2703119042)
re-ACK fa7a40d952ad7588f45f6229aeae754b02fdfb55
See [prior ACK](https://github.com/bitcoin/bitcoin/pull/32074#pullrequestreview-2700285606) for high level take on the change.
New changes:
* Made error message [more helpful](https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2004389857).
* Rebased.
(https://github.com/bitcoin/bitcoin/pull/32074#pullrequestreview-2703119042)
re-ACK fa7a40d952ad7588f45f6229aeae754b02fdfb55
See [prior ACK](https://github.com/bitcoin/bitcoin/pull/32074#pullrequestreview-2700285606) for high level take on the change.
New changes:
* Made error message [more helpful](https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2004389857).
* Rebased.
💬 GregTonoski commented on issue "bitcoind shouldn't fail to progress with synchronization: endless [leveldb] Generated table ... logs":
(https://github.com/bitcoin/bitcoin/issues/31882#issuecomment-2740788385)
> so clearly no deadlock or livelock.
If neither dead- nor live-lock then the algorithm may be slow and suboptimal (bottleneck). There are up to 4 MB of data that need to be downloaded and written to disk per block. On top of it, a fraction of the data may need to be read and written as a "chainstate" index (UTXO set). Apparently, the current algorithm results in orders of magnitude higher use of a disk. I suppose LSM-tree may drive the excessive use of a disk: entries duplication, multiple res
...
(https://github.com/bitcoin/bitcoin/issues/31882#issuecomment-2740788385)
> so clearly no deadlock or livelock.
If neither dead- nor live-lock then the algorithm may be slow and suboptimal (bottleneck). There are up to 4 MB of data that need to be downloaded and written to disk per block. On top of it, a fraction of the data may need to be read and written as a "chainstate" index (UTXO set). Apparently, the current algorithm results in orders of magnitude higher use of a disk. I suppose LSM-tree may drive the excessive use of a disk: entries duplication, multiple res
...
📝 sr-gi opened a pull request: "refactor: Simplifies ProcessMessage for NetMsgType::TX"
(https://github.com/bitcoin/bitcoin/pull/32104)
The processing of a received transaction defines several variables that are not consistently used, defaulting to calling same longer method multiple times. Simplify it so it is easier to read and less error-prone
(https://github.com/bitcoin/bitcoin/pull/32104)
The processing of a received transaction defines several variables that are not consistently used, defaulting to calling same longer method multiple times. Simplify it so it is easier to read and less error-prone
💬 maflcko commented on issue "Incorrect balance when dealing with coinbase UTXOs that will be mature on `h + 1`":
(https://github.com/bitcoin/bitcoin/issues/32098#issuecomment-2740826584)
So I guess this can be closed, or the code could be updated with the rationale as docs?
(https://github.com/bitcoin/bitcoin/issues/32098#issuecomment-2740826584)
So I guess this can be closed, or the code could be updated with the rationale as docs?
💬 ryanofsky commented on pull request "Multiprocess bitcoin":
(https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-2740834776)
re: https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-2738635146
> In this multiprocess taxonomy, for separation-of-concerns and or separation-of-roles support, I was expecting that the default would be for the `node` _not_ to start a `wallet` at all, _instead_ having the `gui` start the `wallet` in addition to the `node` that it already starts?
What actually happens in this PR is that the gui starts a node process and the node starts a wallet process. This is done to keep this PR
...
(https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-2740834776)
re: https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-2738635146
> In this multiprocess taxonomy, for separation-of-concerns and or separation-of-roles support, I was expecting that the default would be for the `node` _not_ to start a `wallet` at all, _instead_ having the `gui` start the `wallet` in addition to the `node` that it already starts?
What actually happens in this PR is that the gui starts a node process and the node starts a wallet process. This is done to keep this PR
...
💬 maflcko commented on pull request "refactor: Simplifies ProcessMessage for NetMsgType::TX":
(https://github.com/bitcoin/bitcoin/pull/32104#discussion_r2005896696)
not sure. This seems like a pessimization that makes it harder to use the type-safe Txid and Wtxid in RelayTransaction
It would be better to make this code type-safe, to avoid having to touch it again in the future.
(https://github.com/bitcoin/bitcoin/pull/32104#discussion_r2005896696)
not sure. This seems like a pessimization that makes it harder to use the type-safe Txid and Wtxid in RelayTransaction
It would be better to make this code type-safe, to avoid having to touch it again in the future.
💬 sipa commented on pull request "[IBD] flush UXTOs in bigger batches":
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2740896169)
Maybe it's a better approach to scale the (default) batch size in function of the dbcache itself? That way, people who have configured their node to run with a low memory footprint to begin with will have relatively small flushing spikes, but people who run on beefy systems and thus likely can tolerate bigger spikes too get to enjoy the performance benefit those may be provide?
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2740896169)
Maybe it's a better approach to scale the (default) batch size in function of the dbcache itself? That way, people who have configured their node to run with a low memory footprint to begin with will have relatively small flushing spikes, but people who run on beefy systems and thus likely can tolerate bigger spikes too get to enjoy the performance benefit those may be provide?
💬 mzumsande commented on issue "test: intermittent issue in p2p_1p1c_network.py":
(https://github.com/bitcoin/bitcoin/issues/31946#issuecomment-2740911853)
Ok thanks - then it looks like there is no actual issue with p2p_1p1c_network.py to fix anymore.
> The goal of the test-each-commit is to check for bisect errors on the original commits.
Would it be possible to first rebase the PR branch onto current master, and only then do the test-each-commit procedure from there? That way, inconsistencies inside amongst the commits of the PR would still be found (maybe even more because silent conflicts that are only there in intermediate commits would now
...
(https://github.com/bitcoin/bitcoin/issues/31946#issuecomment-2740911853)
Ok thanks - then it looks like there is no actual issue with p2p_1p1c_network.py to fix anymore.
> The goal of the test-each-commit is to check for bisect errors on the original commits.
Would it be possible to first rebase the PR branch onto current master, and only then do the test-each-commit procedure from there? That way, inconsistencies inside amongst the commits of the PR would still be found (maybe even more because silent conflicts that are only there in intermediate commits would now
...
💬 sr-gi commented on pull request "refactor: Simplifies ProcessMessage for NetMsgType::TX":
(https://github.com/bitcoin/bitcoin/pull/32104#discussion_r2005955443)
If that is the case, shouldn't we be enforcing RelayTransaction to take Txid and Wtxid instead of uint256?
(https://github.com/bitcoin/bitcoin/pull/32104#discussion_r2005955443)
If that is the case, shouldn't we be enforcing RelayTransaction to take Txid and Wtxid instead of uint256?
🤔 mzumsande reviewed a pull request: "test: Fix intermittent issue in p2p_orphan_handling.py"
(https://github.com/bitcoin/bitcoin/pull/32092#pullrequestreview-2703277956)
Code Review ACK fa310cc6f499e3e3dd58dc382e04e9db3f02bc3b
(https://github.com/bitcoin/bitcoin/pull/32092#pullrequestreview-2703277956)
Code Review ACK fa310cc6f499e3e3dd58dc382e04e9db3f02bc3b
✅ maflcko closed an issue: "test: intermittent issue in p2p_1p1c_network.py"
(https://github.com/bitcoin/bitcoin/issues/31946)
(https://github.com/bitcoin/bitcoin/issues/31946)
💬 maflcko commented on issue "test: intermittent issue in p2p_1p1c_network.py":
(https://github.com/bitcoin/bitcoin/issues/31946#issuecomment-2740929430)
Yeah, let's continue review/discussion in one place ( #32103)
(https://github.com/bitcoin/bitcoin/issues/31946#issuecomment-2740929430)
Yeah, let's continue review/discussion in one place ( #32103)
💬 luisschwab commented on issue "Incorrect balance when dealing with coinbase UTXOs that will be mature on `h + 1`":
(https://github.com/bitcoin/bitcoin/issues/32098#issuecomment-2740940036)
Shouldn't these concerns be separated (transaction building and transaction broadcasting)?
(https://github.com/bitcoin/bitcoin/issues/32098#issuecomment-2740940036)
Shouldn't these concerns be separated (transaction building and transaction broadcasting)?
💬 maflcko commented on pull request "ci: run test-each-commit on merge to master":
(https://github.com/bitcoin/bitcoin/pull/32103#issuecomment-2740984496)
> I think what you want is to checkout each old commit and then merge it (inside the CI task) with master. Not sure how to achieve it otherwise. Maybe you can share your steps to reproduce with `git`?
To clarify, my suggestion would be to modify the `git rebase --exec 'run_test' base` into `git rebase --exec 'git log -1 && git merge --no-commit master && echo run_test && git merge --abort' base`. However, I haven't tested this.
(https://github.com/bitcoin/bitcoin/pull/32103#issuecomment-2740984496)
> I think what you want is to checkout each old commit and then merge it (inside the CI task) with master. Not sure how to achieve it otherwise. Maybe you can share your steps to reproduce with `git`?
To clarify, my suggestion would be to modify the `git rebase --exec 'run_test' base` into `git rebase --exec 'git log -1 && git merge --no-commit master && echo run_test && git merge --abort' base`. However, I haven't tested this.
💬 maflcko commented on issue "Incorrect balance when dealing with coinbase UTXOs that will be mature on `h + 1`":
(https://github.com/bitcoin/bitcoin/issues/32098#issuecomment-2740994889)
The wallet generally is a bit conservative when it comes to creating transactions that may not propagate well. (For example, it doesn't yet offer fullrbf bumps). Of course, anyone is free to manually create the wanted transaction early and propagate it manually.
(https://github.com/bitcoin/bitcoin/issues/32098#issuecomment-2740994889)
The wallet generally is a bit conservative when it comes to creating transactions that may not propagate well. (For example, it doesn't yet offer fullrbf bumps). Of course, anyone is free to manually create the wanted transaction early and propagate it manually.
💬 maflcko commented on pull request "refactor: Simplifies ProcessMessage for NetMsgType::TX":
(https://github.com/bitcoin/bitcoin/pull/32104#discussion_r2006005124)
Yes. Alternatively, at a minimum the `txid` and `wtxid` types should be adjusted here. Otherwise, this refactor may cause a silent merge conflict with the type-safety refactor.
(https://github.com/bitcoin/bitcoin/pull/32104#discussion_r2006005124)
Yes. Alternatively, at a minimum the `txid` and `wtxid` types should be adjusted here. Otherwise, this refactor may cause a silent merge conflict with the type-safety refactor.
👍 vasild approved a pull request: "multiprocess: Add bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2703458243)
ACK 418236c106e32abd7357551d309f8e6d1e494f72
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2703458243)
ACK 418236c106e32abd7357551d309f8e6d1e494f72
💬 l0rinc commented on pull request "[IBD] batch block reads/writes during `AutoFile` serialization":
(https://github.com/bitcoin/bitcoin/pull/31551#discussion_r2006064197)
> This could be an assume
I'm fine with both, thanks, changed reader & writer.
> This could be an assume, given that the program can continue normally, albeit it is a pessimization or possible OOM on low memory machines?
You mean reading the whole block into memory could be the last straw?
Can you please detail the pessimization scenario?
(https://github.com/bitcoin/bitcoin/pull/31551#discussion_r2006064197)
> This could be an assume
I'm fine with both, thanks, changed reader & writer.
> This could be an assume, given that the program can continue normally, albeit it is a pessimization or possible OOM on low memory machines?
You mean reading the whole block into memory could be the last straw?
Can you please detail the pessimization scenario?
👍 vasild approved a pull request: "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods"
(https://github.com/bitcoin/bitcoin/pull/31785#pullrequestreview-2703488792)
ACK cc1001f3bf17b31512c05fb359e09483a07fb2a3
(https://github.com/bitcoin/bitcoin/pull/31785#pullrequestreview-2703488792)
ACK cc1001f3bf17b31512c05fb359e09483a07fb2a3
💬 ryanofsky commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2741144182)
re: https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2738349197
Thanks, if you don't think a separate repository for C bindings would be good, or worth the tradeoffs, that's fine. I was just excited about the idea because I realized with cmake config modules it would be easy to implement technically, and it seemed like a natural starting point to experiment with splitting the project up into different repositories while being able to share utilities and infrastructure.
Just to ex
...
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2741144182)
re: https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2738349197
Thanks, if you don't think a separate repository for C bindings would be good, or worth the tradeoffs, that's fine. I was just excited about the idea because I realized with cmake config modules it would be easy to implement technically, and it seemed like a natural starting point to experiment with splitting the project up into different repositories while being able to share utilities and infrastructure.
Just to ex
...