💬 Sjors commented on pull request "wallet: skip R-value signature grinding for external signers":
(https://github.com/bitcoin/bitcoin/pull/26032#discussion_r1116029872)
Though perhaps it's overkill, because `IsWalletFlagSet` is just `m_wallet_flags & flag`.
(https://github.com/bitcoin/bitcoin/pull/26032#discussion_r1116029872)
Though perhaps it's overkill, because `IsWalletFlagSet` is just `m_wallet_flags & flag`.
✅ MarcoFalke closed a pull request: "refactor: Avoid UniValue copies"
(https://github.com/bitcoin/bitcoin/pull/25429)
(https://github.com/bitcoin/bitcoin/pull/25429)
💬 Sjors commented on pull request "contrib: Improve verify-commits.py to work with maintainers leaving":
(https://github.com/bitcoin/bitcoin/pull/27058#issuecomment-1442240383)
@petertodd in practice things should be fine if there's at least a few days between when a maintainer last merges something and the date we put in a CSV to track when they are no longer authorised. Maybe a bit more if there's a congestion delay in when the timestamp gets included. In the case of Wladimir there was a full year between his last merge and revoking his key.
(https://github.com/bitcoin/bitcoin/pull/27058#issuecomment-1442240383)
@petertodd in practice things should be fine if there's at least a few days between when a maintainer last merges something and the date we put in a CSV to track when they are no longer authorised. Maybe a bit more if there's a congestion delay in when the timestamp gets included. In the case of Wladimir there was a full year between his last merge and revoking his key.
💬 Sjors commented on pull request "assumeutxo: keep cache when flushing snapshot (#17487 followup)":
(https://github.com/bitcoin/bitcoin/pull/27008#issuecomment-1442246762)
re-utACK 937016b1917a0a07fa579c96fc797237f5027cd1
(https://github.com/bitcoin/bitcoin/pull/27008#issuecomment-1442246762)
re-utACK 937016b1917a0a07fa579c96fc797237f5027cd1
💬 Sjors commented on pull request "assumeutxo: keep cache when flushing snapshot (#17487 followup)":
(https://github.com/bitcoin/bitcoin/pull/27008#discussion_r1116102760)
Same question about `AbortNode` vs. `error` though.
(https://github.com/bitcoin/bitcoin/pull/27008#discussion_r1116102760)
Same question about `AbortNode` vs. `error` though.
💬 Sjors commented on pull request "assumeutxo: background validation completion":
(https://github.com/bitcoin/bitcoin/pull/25740#issuecomment-1442260424)
I'd like to test this with `loadtxoutset`. I wonder if I just cherry-pick a few things from #15606 to do that myself, or a more rigorous rebase is needed...
(https://github.com/bitcoin/bitcoin/pull/25740#issuecomment-1442260424)
I'd like to test this with `loadtxoutset`. I wonder if I just cherry-pick a few things from #15606 to do that myself, or a more rigorous rebase is needed...
💬 furszy commented on pull request "wallet: skip R-value signature grinding for external signers":
(https://github.com/bitcoin/bitcoin/pull/26032#discussion_r1116121819)
yeah, I think that the only place that worth is inside `AvailableCoins`. And not only because of the overhead that introduces (it's one "AND" operation on an atomic variable per wallet UTXO), I was more thinking about the code structure and what we should do if we want to decrease the `cs_wallet` lock contention there.
still, this is still a tiny nit from a person that is thinking about future stuff.
(https://github.com/bitcoin/bitcoin/pull/26032#discussion_r1116121819)
yeah, I think that the only place that worth is inside `AvailableCoins`. And not only because of the overhead that introduces (it's one "AND" operation on an atomic variable per wallet UTXO), I was more thinking about the code structure and what we should do if we want to decrease the `cs_wallet` lock contention there.
still, this is still a tiny nit from a person that is thinking about future stuff.
✅ achow101 closed an issue: "rpc: prevent non-zero value OP_RETURN outputs in sendrawtransaction"
(https://github.com/bitcoin/bitcoin/issues/25899)
(https://github.com/bitcoin/bitcoin/issues/25899)
🚀 achow101 merged a pull request: "rpc: Add a parameter to sendrawtransaction which sets a maximum value for unspendable outputs."
(https://github.com/bitcoin/bitcoin/pull/25943)
(https://github.com/bitcoin/bitcoin/pull/25943)
💬 petertodd commented on pull request "contrib: Improve verify-commits.py to work with maintainers leaving":
(https://github.com/bitcoin/bitcoin/pull/27058#issuecomment-1442293610)
> @petertodd in practice things should be fine if there's at least a few days between when a maintainer last merges something and the date we put in a CSV to track when they are no longer authorised. Maybe a bit more if there's a congestion delay in when the timestamp gets included. In the case of Wladimir there was a full year between his last merge and revoking his key.
My comment is about what standard an automated tool should apply. Obviously, if some humans are looking at it and manually
...
(https://github.com/bitcoin/bitcoin/pull/27058#issuecomment-1442293610)
> @petertodd in practice things should be fine if there's at least a few days between when a maintainer last merges something and the date we put in a CSV to track when they are no longer authorised. Maybe a bit more if there's a congestion delay in when the timestamp gets included. In the case of Wladimir there was a full year between his last merge and revoking his key.
My comment is about what standard an automated tool should apply. Obviously, if some humans are looking at it and manually
...
💬 Sjors commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1116158372)
I find the explanation in the function body more clear. Maybe say:
```md
// Because we expect to have received the headers for the IBD chain
// contents before receiving blocks, when the (shared) block manager
// doesn't include a header for this block, it must go to
// to the snapshot chain.
```
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1116158372)
I find the explanation in the function body more clear. Maybe say:
```md
// Because we expect to have received the headers for the IBD chain
// contents before receiving blocks, when the (shared) block manager
// doesn't include a header for this block, it must go to
// to the snapshot chain.
```
💬 Sjors commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1116169293)
IIUC: maybe clarify here that `m_chain.Contains` searches both the snapshot headers _and_ the IBD headers it's built on top of. When a `pblock` exists but is not contained in the chainstate it's a fork, and it's up to the snapshot chainstate to explore those.
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1116169293)
IIUC: maybe clarify here that `m_chain.Contains` searches both the snapshot headers _and_ the IBD headers it's built on top of. When a `pblock` exists but is not contained in the chainstate it's a fork, and it's up to the snapshot chainstate to explore those.
💬 Sjors commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1116142978)
That seems like a good idea, also because `GetSnapshotBaseHeight` will deference the result to get a height. (Though it won't crash because it checks if the result is a `nullptr`.)
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1116142978)
That seems like a good idea, also because `GetSnapshotBaseHeight` will deference the result to get a height. (Though it won't crash because it checks if the result is a `nullptr`.)
💬 Sjors commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1116123753)
Neither are used here, because `GetSnapshotBaseBlock` is only used by `GetSnapshotBaseHeight`. But they are used in the final PR, so I'm fine with introducing them here.
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1116123753)
Neither are used here, because `GetSnapshotBaseBlock` is only used by `GetSnapshotBaseHeight`. But they are used in the final PR, so I'm fine with introducing them here.
💬 TheCharlatan commented on pull request "Fix various libbitcoinkernel DLL build problems":
(https://github.com/bitcoin/bitcoin/pull/27146#discussion_r1116185034)
Nice, I applied theuni@1733d01 and:
```
objdump -p install_dir/bin/bitcoin-chainstate.exe | grep dll
DLL Name: libbitcoinkernel-0.dll
DLL Name: ADVAPI32.dll
DLL Name: KERNEL32.dll
DLL Name: msvcrt.dll
```
:smile:
(https://github.com/bitcoin/bitcoin/pull/27146#discussion_r1116185034)
Nice, I applied theuni@1733d01 and:
```
objdump -p install_dir/bin/bitcoin-chainstate.exe | grep dll
DLL Name: libbitcoinkernel-0.dll
DLL Name: ADVAPI32.dll
DLL Name: KERNEL32.dll
DLL Name: msvcrt.dll
```
:smile:
⚠️ Sjors opened an issue: "test: p2p_message_capture.py fails with undefined sanitizer"
(https://github.com/bitcoin/bitcoin/issues/27149)
On Ubuntu 22.10
Relevant configure variable: `--with-sanitizers=undefined`
```
$ test/functional/p2p_message_capture.py
2023-02-23T19:55:57.251000Z TestFramework (INFO): PRNG seed is: 8604348948412116696
2023-02-23T19:55:57.251000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_v2mn0yad
2023-02-23T19:55:57.666000Z TestFramework (INFO): Stopping nodes
Traceback (most recent call last):
File "test/functional/p2p_message_capture.py", line 72, in <module>
...
(https://github.com/bitcoin/bitcoin/issues/27149)
On Ubuntu 22.10
Relevant configure variable: `--with-sanitizers=undefined`
```
$ test/functional/p2p_message_capture.py
2023-02-23T19:55:57.251000Z TestFramework (INFO): PRNG seed is: 8604348948412116696
2023-02-23T19:55:57.251000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_v2mn0yad
2023-02-23T19:55:57.666000Z TestFramework (INFO): Stopping nodes
Traceback (most recent call last):
File "test/functional/p2p_message_capture.py", line 72, in <module>
...
💬 Sjors commented on issue " Document how to run bitcoin unittests + functional tests with sanitizers":
(https://github.com/bitcoin/bitcoin/issues/17834#issuecomment-1442364207)
Anything left to do here?
Seems mostly a matter of `./configure with-sanitizers=…` and then the test suite runs as normal, albeit slower and more memory hungry.
(https://github.com/bitcoin/bitcoin/issues/17834#issuecomment-1442364207)
Anything left to do here?
Seems mostly a matter of `./configure with-sanitizers=…` and then the test suite runs as normal, albeit slower and more memory hungry.
💬 MarcoFalke commented on issue "test: p2p_message_capture.py fails with undefined sanitizer":
(https://github.com/bitcoin/bitcoin/issues/27149#issuecomment-1442374172)
Did you set the suppression file? (`env|grep UBSAN`?)
(https://github.com/bitcoin/bitcoin/issues/27149#issuecomment-1442374172)
Did you set the suppression file? (`env|grep UBSAN`?)
💬 Sjors commented on issue "test: p2p_message_capture.py fails with undefined sanitizer":
(https://github.com/bitcoin/bitcoin/issues/27149#issuecomment-1442383883)
Which suppression file?
Aha: https://github.com/bitcoin/bitcoin/blob/master/test/sanitizer_suppressions/ubsan
We should point to it in https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#sanitizers
(https://github.com/bitcoin/bitcoin/issues/27149#issuecomment-1442383883)
Which suppression file?
Aha: https://github.com/bitcoin/bitcoin/blob/master/test/sanitizer_suppressions/ubsan
We should point to it in https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#sanitizers
💬 MarcoFalke commented on issue "RPC: gettxoutsetinfo correctly flushes transactions to the coindb, but then does not return any RPC reply, and keeps running":
(https://github.com/bitcoin/bitcoin/issues/25912#issuecomment-1442387196)
Is this still an issue with a recent version of Bitcoin Core? If yes, what are the steps to reproduce?
(https://github.com/bitcoin/bitcoin/issues/25912#issuecomment-1442387196)
Is this still an issue with a recent version of Bitcoin Core? If yes, what are the steps to reproduce?