💬 hebasto commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#issuecomment-2133434672)
> It's not a full drop-in replacement...
Then, perhaps, it's a good chance to avoid `size_t` for parameter and return types in the interface?
See: [Signed and Unsigned Types in Interfaces](https://www.aristeia.com/Papers/C++ReportColumns/sep95.pdf)
(https://github.com/bitcoin/bitcoin/pull/30161#issuecomment-2133434672)
> It's not a full drop-in replacement...
Then, perhaps, it's a good chance to avoid `size_t` for parameter and return types in the interface?
See: [Signed and Unsigned Types in Interfaces](https://www.aristeia.com/Papers/C++ReportColumns/sep95.pdf)
📝 brunoerg opened a pull request: "rpc: net: follow-ups for #30062"
(https://github.com/bitcoin/bitcoin/pull/30183)
- Change `addrman` to reference to const since it isn't modified (https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1612272793).
- Improve documentation of `mapped_as`/`source_mapped_as` in `getrawaddrman` RPC by mentioning that both fields will be only available if asmap flag is set. It is the same message for `mapped_as` field in `getpeerinfo`.
(https://github.com/bitcoin/bitcoin/pull/30183)
- Change `addrman` to reference to const since it isn't modified (https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1612272793).
- Improve documentation of `mapped_as`/`source_mapped_as` in `getrawaddrman` RPC by mentioning that both fields will be only available if asmap flag is set. It is the same message for `mapped_as` field in `getpeerinfo`.
💬 theStack commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1616064751)
Each byte in the output script counts as four weight units, so with the padding method used here, we can only ever increase the weight by multiples of four. If the weight of the unpadded tx is not a multiple of four already (i.e. tx.get_weight() % 4 != 0), it will also not be after the padding and the best we can do is to be not more off than 3 WUs in the worst case.
It would be nice if we could somehow do exact padding with weight unit precision, but to my knowledge there is no general way t
...
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1616064751)
Each byte in the output script counts as four weight units, so with the padding method used here, we can only ever increase the weight by multiples of four. If the weight of the unpadded tx is not a multiple of four already (i.e. tx.get_weight() % 4 != 0), it will also not be after the padding and the best we can do is to be not more off than 3 WUs in the worst case.
It would be nice if we could somehow do exact padding with weight unit precision, but to my knowledge there is no general way t
...
💬 theStack commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1616064960)
We need a running node to create a MiniWallet instance, so unfortunately doing these checks in an unit test wouldn't work. I considered putting it into an existing functional test but wouldn't know which one to pick, so creating a new MiniWallet-specific test where features can tested independently seemed to make the most sense.
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1616064960)
We need a running node to create a MiniWallet instance, so unfortunately doing these checks in an unit test wouldn't work. I considered putting it into an existing functional test but wouldn't know which one to pick, so creating a new MiniWallet-specific test where features can tested independently seemed to make the most sense.
💬 theStack commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1616065138)
Passing `target_weight` with an absolute `fee` should still work fine. There is arguably less reason now to do so as passing a `fee_rate` is likely more convenient for most use-cases, but I don't think we have to disallow padding txs with an absolute fee.
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1616065138)
Passing `target_weight` with an absolute `fee` should still work fine. There is arguably less reason now to do so as passing a `fee_rate` is likely more convenient for most use-cases, but I don't think we have to disallow padding txs with an absolute fee.
💬 theStack commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1616065536)
The actual weight can be larger by at most 3 WUs, [see comment above](https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1616064751).
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1616065536)
The actual weight can be larger by at most 3 WUs, [see comment above](https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1616064751).
✅ kosuodhmwa closed an issue: ""bitcoin-cli" does not exist, while "bitcoind" does in ~/bitcoin/src folder"
(https://github.com/bitcoin/bitcoin/issues/30180)
(https://github.com/bitcoin/bitcoin/issues/30180)
💬 kosuodhmwa commented on issue ""bitcoin-cli" does not exist, while "bitcoind" does in ~/bitcoin/src folder":
(https://github.com/bitcoin/bitcoin/issues/30180#issuecomment-2133532648)
solved, was a mistake of me. thank you anyway :-)
(https://github.com/bitcoin/bitcoin/issues/30180#issuecomment-2133532648)
solved, was a mistake of me. thank you anyway :-)
💬 sipa commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#issuecomment-2133774404)
@hebasto I do prefer to stay compatible with the `std::deque` interface, even if just for matching behavior users would expect.
(https://github.com/bitcoin/bitcoin/pull/30161#issuecomment-2133774404)
@hebasto I do prefer to stay compatible with the `std::deque` interface, even if just for matching behavior users would expect.
👍 itornaza approved a pull request: "consensus: fix `OP_1NEGATE` handling in `CScriptOp`"
(https://github.com/bitcoin/bitcoin/pull/29589#pullrequestreview-2081224349)
tested ACK 0abf50a71b0439ee7f4e94c548b29d726c58477a
Examined all changes from 3fb1b7043a7150f70a4cf1e172968e32c056dc79 up to this commit using `git difftool 3fb1b70 0abf50a` on the changed files. Did a code review on the changes that touches consensus code in`src/script/script.h` as well as the associated test file `test/functional/test_framework/script.py`.
The changes in the python test functions `encode_op_n` and `decode_op_n` are now one to one compatible with the consensus`EncodeOP_N
...
(https://github.com/bitcoin/bitcoin/pull/29589#pullrequestreview-2081224349)
tested ACK 0abf50a71b0439ee7f4e94c548b29d726c58477a
Examined all changes from 3fb1b7043a7150f70a4cf1e172968e32c056dc79 up to this commit using `git difftool 3fb1b70 0abf50a` on the changed files. Did a code review on the changes that touches consensus code in`src/script/script.h` as well as the associated test file `test/functional/test_framework/script.py`.
The changes in the python test functions `encode_op_n` and `decode_op_n` are now one to one compatible with the consensus`EncodeOP_N
...
💬 itornaza commented on pull request "consensus: fix `OP_1NEGATE` handling in `CScriptOp`":
(https://github.com/bitcoin/bitcoin/pull/29589#discussion_r1616232765)
Non-blocking comment: Maybe move the `assert` line at the beginning of this member function so it is even more similar to its `EncodeOP_N` counterpart.
(https://github.com/bitcoin/bitcoin/pull/29589#discussion_r1616232765)
Non-blocking comment: Maybe move the `assert` line at the beginning of this member function so it is even more similar to its `EncodeOP_N` counterpart.
💬 itornaza commented on pull request "test: Add `leaf_version` parameter to `taproot_tree_helper()`":
(https://github.com/bitcoin/bitcoin/pull/29371#issuecomment-2133824938)
trACK 3d4dda0b26d3e003d282be1e30e64055750a792e
(https://github.com/bitcoin/bitcoin/pull/29371#issuecomment-2133824938)
trACK 3d4dda0b26d3e003d282be1e30e64055750a792e
💬 brunoerg commented on pull request "fuzz: wallet: add target for spkm migration":
(https://github.com/bitcoin/bitcoin/pull/29694#issuecomment-2133888537)
cc: @achow101
(https://github.com/bitcoin/bitcoin/pull/29694#issuecomment-2133888537)
cc: @achow101
🤔 BrandonOdiwuor reviewed a pull request: "net: log connections failures via SOCKS5 with less severity"
(https://github.com/bitcoin/bitcoin/pull/30064#pullrequestreview-2081391076)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30064#pullrequestreview-2081391076)
Concept ACK
👋 0xBEEFCAF3's pull request is ready for review: "Reenable OP_CAT"
(https://github.com/bitcoin/bitcoin/pull/29247)
(https://github.com/bitcoin/bitcoin/pull/29247)
💬 fjahr commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2134106601)
Here is a BIP PR for Testnet 4: https://github.com/bitcoin/bips/pull/1601
I think the written specification needs to be a BIP to be considered meaningful in the long run. If I just put it in a gist or something like that it depends on me alone to make changes should they become necessary for example. I would rather have it be managed by the community if the written specification is what people turn to.
The PR still might get changed obviously but I will update the BIP PR accordingly.
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2134106601)
Here is a BIP PR for Testnet 4: https://github.com/bitcoin/bips/pull/1601
I think the written specification needs to be a BIP to be considered meaningful in the long run. If I just put it in a gist or something like that it depends on me alone to make changes should they become necessary for example. I would rather have it be managed by the community if the written specification is what people turn to.
The PR still might get changed obviously but I will update the BIP PR accordingly.
💬 cryptoquick commented on pull request "Extend signetchallenge to set target block spacing":
(https://github.com/bitcoin/bitcoin/pull/29365#issuecomment-2134338774)
Tested ACK
Works great against the Mutinynet node with the provided configuration, and the approach seems appropriate
(https://github.com/bitcoin/bitcoin/pull/29365#issuecomment-2134338774)
Tested ACK
Works great against the Mutinynet node with the provided configuration, and the approach seems appropriate
💬 Sjors commented on pull request "Drop -dbcache limit":
(https://github.com/bitcoin/bitcoin/pull/28358#issuecomment-2134456888)
@BenWestgate thanks for the analysis. If we want to add a recommendation to the help text, I suggest we do that in another PR though. It's probably going to take multiple people doing benchmark on wildly different machines.
Checking against RAM can also be done in a followup.
I rebased and changed the .md file name.
(https://github.com/bitcoin/bitcoin/pull/28358#issuecomment-2134456888)
@BenWestgate thanks for the analysis. If we want to add a recommendation to the help text, I suggest we do that in another PR though. It's probably going to take multiple people doing benchmark on wildly different machines.
Checking against RAM can also be done in a followup.
I rebased and changed the .md file name.
💬 stratospher commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1616695926)
done.
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1616695926)
done.
💬 stratospher commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1616701338)
> I still think this can be simplified. initiate_v2_handshake is going to be called, asynchronously, when creating this object. You can reduce a lot of the boilerplate, and get rid of magic_sent, as long as you do the last bit manually on the test (or just create another method, like continue_v2_handshake).
yes, that can be done since the last bit is being sent from `MainThread` anyways.
before:
1. send both 4 bytes network magic (first 4 bytes of ellswift) - `NetworkThread`
2. remainin
...
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1616701338)
> I still think this can be simplified. initiate_v2_handshake is going to be called, asynchronously, when creating this object. You can reduce a lot of the boilerplate, and get rid of magic_sent, as long as you do the last bit manually on the test (or just create another method, like continue_v2_handshake).
yes, that can be done since the last bit is being sent from `MainThread` anyways.
before:
1. send both 4 bytes network magic (first 4 bytes of ellswift) - `NetworkThread`
2. remainin
...
💬 stratospher commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#issuecomment-2134500918)
thank you for the helpful reviews @sr-gi!
I've updated the PR:
- `EARLY_KEY_RESPONSE` test doesn't have `magic_sent` now and the `initial_v2_handshake()` function is cleaner
- `EARLY_KEY_RESPONSE` test does sending of 4 bytes ellswift (which match network magic) + remaining ellswift and garbage bytes on `MainThread` - both are done manually now
- Added v2 handshake timeout log in 7d07daa
> What is weird is that the test is expecting a specific log message, which seems to also mat
...
(https://github.com/bitcoin/bitcoin/pull/29431#issuecomment-2134500918)
thank you for the helpful reviews @sr-gi!
I've updated the PR:
- `EARLY_KEY_RESPONSE` test doesn't have `magic_sent` now and the `initial_v2_handshake()` function is cleaner
- `EARLY_KEY_RESPONSE` test does sending of 4 bytes ellswift (which match network magic) + remaining ellswift and garbage bytes on `MainThread` - both are done manually now
- Added v2 handshake timeout log in 7d07daa
> What is weird is that the test is expecting a specific log message, which seems to also mat
...