💬 i-am-yuvi commented on pull request "test: add MAX_DISCONNECTED_TX_POOL_BYTES, chainlimits coverage":
(https://github.com/bitcoin/bitcoin/pull/32516#issuecomment-2898614400)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32516#issuecomment-2898614400)
Concept ACK
🤔 i-am-yuvi reviewed a pull request: "test: add MAX_DISCONNECTED_TX_POOL_BYTES, chainlimits coverage"
(https://github.com/bitcoin/bitcoin/pull/32516#pullrequestreview-2858508507)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32516#pullrequestreview-2858508507)
Concept ACK
🤔 i-am-yuvi reviewed a pull request: "test: add MAX_DISCONNECTED_TX_POOL_BYTES, chainlimits coverage"
(https://github.com/bitcoin/bitcoin/pull/32516#pullrequestreview-2858551944)
tACK 2b202e9db56487e658fc41089178f31ef4259a0d
- `DisconnectTip()` & reconnect via `invalidateblock` and `reconsiderblock`
- `LimitMemoryUsage()`: exercised by flooding > 20 MB
- `test_chainlimits_exceeded`
Logs
```
2025-05-21T16:57:42.023000Z TestFramework (INFO): PRNG seed is: 4085491027579870526
2025-05-21T16:57:42.024000Z TestFramework (INFO): Initializing test directory /var/folders/jb/wlbrz0t95vl58wzxjt75wqmh0000gn/T/bitcoin_func_test_jb27si_k
2025-05-21T16:57:42.337000Z TestFr
...
(https://github.com/bitcoin/bitcoin/pull/32516#pullrequestreview-2858551944)
tACK 2b202e9db56487e658fc41089178f31ef4259a0d
- `DisconnectTip()` & reconnect via `invalidateblock` and `reconsiderblock`
- `LimitMemoryUsage()`: exercised by flooding > 20 MB
- `test_chainlimits_exceeded`
Logs
```
2025-05-21T16:57:42.023000Z TestFramework (INFO): PRNG seed is: 4085491027579870526
2025-05-21T16:57:42.024000Z TestFramework (INFO): Initializing test directory /var/folders/jb/wlbrz0t95vl58wzxjt75wqmh0000gn/T/bitcoin_func_test_jb27si_k
2025-05-21T16:57:42.337000Z TestFr
...
📝 dergoegge opened a pull request: "allocators: Apply manual ASan poisoning to `PoolResource`"
(https://github.com/bitcoin/bitcoin/pull/32581)
Currently ASan will not detect use-after-free issues for memory allocated by a `PoolResource`. This is because ASan is only aware of the memory chunks allocated by `PoolResource` but not the individual "sub-chunks" within.
E.g. this test will not produce an ASan error even though the referenced coin has been deallocated:
```c++
iff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp
index c46144b34b..aa6ca15ce1 100644
--- a/src/test/coins_tests.cpp
+++ b/src/test/coins_tests.cp
...
(https://github.com/bitcoin/bitcoin/pull/32581)
Currently ASan will not detect use-after-free issues for memory allocated by a `PoolResource`. This is because ASan is only aware of the memory chunks allocated by `PoolResource` but not the individual "sub-chunks" within.
E.g. this test will not produce an ASan error even though the referenced coin has been deallocated:
```c++
iff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp
index c46144b34b..aa6ca15ce1 100644
--- a/src/test/coins_tests.cpp
+++ b/src/test/coins_tests.cp
...
👋 rkrux's pull request is ready for review: "wallet, test: best block locator matches scan state follow-ups"
(https://github.com/bitcoin/bitcoin/pull/32580)
(https://github.com/bitcoin/bitcoin/pull/32580)
📝 dergoegge converted_to_draft a pull request: "allocators: Apply manual ASan poisoning to `PoolResource`"
(https://github.com/bitcoin/bitcoin/pull/32581)
Currently ASan will not detect use-after-free issues for memory allocated by a `PoolResource`. This is because ASan is only aware of the memory chunks allocated by `PoolResource` but not the individual "sub-chunks" within.
E.g. this test will not produce an ASan error even though the referenced coin has been deallocated:
```c++
diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp
index c46144b34b..aa6ca15ce1 100644
--- a/src/test/coins_tests.cpp
+++ b/src/test/coins_tests.c
...
(https://github.com/bitcoin/bitcoin/pull/32581)
Currently ASan will not detect use-after-free issues for memory allocated by a `PoolResource`. This is because ASan is only aware of the memory chunks allocated by `PoolResource` but not the individual "sub-chunks" within.
E.g. this test will not produce an ASan error even though the referenced coin has been deallocated:
```c++
diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp
index c46144b34b..aa6ca15ce1 100644
--- a/src/test/coins_tests.cpp
+++ b/src/test/coins_tests.c
...
💬 dergoegge commented on pull request "allocators: Apply manual ASan poisoning to `PoolResource`":
(https://github.com/bitcoin/bitcoin/pull/32581#issuecomment-2898738831)
Draft until I sort out the CI failures
(https://github.com/bitcoin/bitcoin/pull/32581#issuecomment-2898738831)
Draft until I sort out the CI failures
💬 rkrux commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#issuecomment-2898747996)
> It looks like there are some small review comments above that could be followed up, but with 3 acks I think it would be good to merge this shortly and let smaller improvements can be made later. I plan to merge this soon after testing locally.
I did the follow-ups in #32580.
(https://github.com/bitcoin/bitcoin/pull/30221#issuecomment-2898747996)
> It looks like there are some small review comments above that could be followed up, but with 3 acks I think it would be good to merge this shortly and let smaller improvements can be made later. I plan to merge this soon after testing locally.
I did the follow-ups in #32580.
💬 achow101 commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#issuecomment-2898773591)
Rebased, ready for review.
(https://github.com/bitcoin/bitcoin/pull/32523#issuecomment-2898773591)
Rebased, ready for review.
👋 achow101's pull request is ready for review: "wallet: Remove watchonly behavior and isminetypes"
(https://github.com/bitcoin/bitcoin/pull/32523)
(https://github.com/bitcoin/bitcoin/pull/32523)
👋 achow101's pull request is ready for review: "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet"
(https://github.com/bitcoin/bitcoin/pull/32489)
(https://github.com/bitcoin/bitcoin/pull/32489)
💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#issuecomment-2898781091)
Rebased, ready for review
(https://github.com/bitcoin/bitcoin/pull/32489#issuecomment-2898781091)
Rebased, ready for review
💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2100870613)
Done
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2100870613)
Done
💬 pinheadmz commented on pull request "rpc: Round verificationprogress to 1 for a recent tip":
(https://github.com/bitcoin/bitcoin/pull/32528#discussion_r2100887156)
fa53098472521de9d5b3cc42983043c240b7c321
Why is this here and not in the next block that is already guarded by `pindex_was_in_chain`? `to_mark_failed` and `m_chainman` are still in scope.
(https://github.com/bitcoin/bitcoin/pull/32528#discussion_r2100887156)
fa53098472521de9d5b3cc42983043c240b7c321
Why is this here and not in the next block that is already guarded by `pindex_was_in_chain`? `to_mark_failed` and `m_chainman` are still in scope.
💬 pinheadmz commented on pull request "rpc: Round verificationprogress to 1 for a recent tip":
(https://github.com/bitcoin/bitcoin/pull/32528#issuecomment-2898819643)
One thing that's a little weird about this is that when I open regtest after a few days...
```
--> bccli -regtest -getinfo
Chain: regtest
Blocks: 202
Headers: 202
Verification progress: ▒▒▒▒▒▒░░░░░░░░░░░░░░░ 27.7195%
```
Of course after generating 1 block at time=now, it snaps to 100%
(https://github.com/bitcoin/bitcoin/pull/32528#issuecomment-2898819643)
One thing that's a little weird about this is that when I open regtest after a few days...
```
--> bccli -regtest -getinfo
Chain: regtest
Blocks: 202
Headers: 202
Verification progress: ▒▒▒▒▒▒░░░░░░░░░░░░░░░ 27.7195%
```
Of course after generating 1 block at time=now, it snaps to 100%
💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2100880884)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2099737369
execvp is documented to return -1 if there is an error (https://linux.die.net/man/3/execvp and https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/execvp-wexecvp) so if it returns something else I think it is better to abort and not just keep going.
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2100880884)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2099737369
execvp is documented to return -1 if there is an error (https://linux.die.net/man/3/execvp and https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/execvp-wexecvp) so if it returns something else I think it is better to abort and not just keep going.
💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2100888510)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2099742506
Yes I used to refer to them as NUL terminated strings, until I came across https://en.wikipedia.org/wiki/Null-terminated_string and decided there wasn't any real ambiguity and null was probably easier to read. But would be ok changing this if others would prefer
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2100888510)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2099742506
Yes I used to refer to them as NUL terminated strings, until I came across https://en.wikipedia.org/wiki/Null-terminated_string and decided there wasn't any real ambiguity and null was probably easier to read. But would be ok changing this if others would prefer
🤔 ryanofsky reviewed a pull request: "multiprocess: Add bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2858683133)
Thanks for reviewing again!
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2858683133)
Thanks for reviewing again!
💬 davidgumberg commented on pull request "rpc: Round verificationprogress to 1 for a recent tip":
(https://github.com/bitcoin/bitcoin/pull/32528#discussion_r2100898080)
Am I correct in understanding that `handleNotifyBlockTip` takes a lock now where it didn't before?
(https://github.com/bitcoin/bitcoin/pull/32528#discussion_r2100898080)
Am I correct in understanding that `handleNotifyBlockTip` takes a lock now where it didn't before?
💬 rkrux commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#issuecomment-2898830691)
This PR comes at a right time. I'm prioritising reviewing this one because it removes `isminetype`.
#27286: The new wallet unspents model in memory is using `isminetype` currently and I'd prefer to not have it checked into master only to be removed subsequently. This diff should also ease the review of the other PR.
https://github.com/bitcoin/bitcoin/pull/27286/commits/d637af8a9786e38f1ba140ed828a3d5009e62926#diff-d41d68c5a65d67956c91b33ca86da7df1981d84a0b15b4a186deea566563fed5R372
(https://github.com/bitcoin/bitcoin/pull/32523#issuecomment-2898830691)
This PR comes at a right time. I'm prioritising reviewing this one because it removes `isminetype`.
#27286: The new wallet unspents model in memory is using `isminetype` currently and I'd prefer to not have it checked into master only to be removed subsequently. This diff should also ease the review of the other PR.
https://github.com/bitcoin/bitcoin/pull/27286/commits/d637af8a9786e38f1ba140ed828a3d5009e62926#diff-d41d68c5a65d67956c91b33ca86da7df1981d84a0b15b4a186deea566563fed5R372