💬 instagibbs commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2740749545)
reviewed via `git range-diff master 72a97c0a07ea6e5a95ab37c8d95e1ea02cff8e92 1601906941fa559ebbee7898453fa77f4606ad38`
> Ref objects are now allowed to outlive TxGraph
Can this get surfaced in the fuzz coverage with something that would previously fail?
(https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2740749545)
reviewed via `git range-diff master 72a97c0a07ea6e5a95ab37c8d95e1ea02cff8e92 1601906941fa559ebbee7898453fa77f4606ad38`
> Ref objects are now allowed to outlive TxGraph
Can this get surfaced in the fuzz coverage with something that would previously fail?
💬 EthanHeilman commented on pull request "tests: improves tapscript unit tests":
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2740760807)
@instagibbs Any changes you want me to make?
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2740760807)
@instagibbs Any changes you want me to make?
💬 maflcko commented on pull request "contrib: Make deterministic-coverage error messages more readable":
(https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2005845404)
thx, done!
(https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2005845404)
thx, done!
💬 Eunovo commented on pull request "[IBD] flush UXTOs in bigger batches":
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2740766215)
I used `valgrind --tool=massif` [valgrind massif](https://valgrind.org/docs/manual/ms-manual.html) to profile memory usage of bitcoin-core IBD(840002 to 850000) for 16MiB and 64MiB batch sizes.
## Memory Usage Tests
### Method
```
mkdir demo
build/bin/bitcoind -datadir=demo -stopatheight=1
build/bin/bitcoind -datadir=demo -daemon -blocksonly=1 -stopatheight=840001
build/bin/bitcoin-cli -datadir=demo -rpcclienttimeout=0 loadtxoutset ~/utxo-840000.dat
mkdir demo-backup
cp -r demo/* de
...
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2740766215)
I used `valgrind --tool=massif` [valgrind massif](https://valgrind.org/docs/manual/ms-manual.html) to profile memory usage of bitcoin-core IBD(840002 to 850000) for 16MiB and 64MiB batch sizes.
## Memory Usage Tests
### Method
```
mkdir demo
build/bin/bitcoind -datadir=demo -stopatheight=1
build/bin/bitcoind -datadir=demo -daemon -blocksonly=1 -stopatheight=840001
build/bin/bitcoin-cli -datadir=demo -rpcclienttimeout=0 loadtxoutset ~/utxo-840000.dat
mkdir demo-backup
cp -r demo/* de
...
💬 instagibbs commented on pull request "tests: improves tapscript unit tests":
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2740767653)
Let me take a deeper look
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2740767653)
Let me take a deeper look
💬 maflcko commented on issue "test: No unit test covers BIP342 tapscript signatures":
(https://github.com/bitcoin/bitcoin/issues/32012#issuecomment-2740769810)
related to https://github.com/bitcoin/bitcoin/pull/31576 ?
(https://github.com/bitcoin/bitcoin/issues/32012#issuecomment-2740769810)
related to https://github.com/bitcoin/bitcoin/pull/31576 ?
👍 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.